-
Notifications
You must be signed in to change notification settings - Fork 30
Use canonical management URLs for HTTP payloads #237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+364
−37
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0247ca9
Fix canonical management payload URLs
andystaples a620827
Merge branch 'main' into andystaples-fix-management-payload-urls
berndverst e0aa562
Address management URL review feedback
andystaples b75223c
Merge branch 'main' into andystaples-fix-management-payload-urls
andystaples 79f1ab5
Validate forwarded management URL origins
andystaples File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Durable extension gates forwarded-origin handling behind
extensions.durableTask.httpSettings.useForwardedHost, whose default isfalse. This helper instead trustsForwarded/X-Forwarded-*on every request, andcreate_check_status_response()then copies that origin (plus the management system key) into both the response body andLocation. For example, anX-Forwarded-Host: attacker.examplerequest now producesLocation: https://attacker.example/...?...code=<system-key>even when the host option is disabled. Please pass an explicit host-provided opt-in/trust flag and gate both header families; otherwise retain the origin fromrequest.url. Add a default-off regression test as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by adding an optional host-provided
useForwardedHostflag that defaults tofalse; without opt-in, both forwarded-header families are ignored and the origin comes fromrequest.url. I also added a regression test showing a spoofedX-Forwarded-Hostcannot redirect the check-status payload orLocation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still incomplete at
e0aa562. The current Durable extensiondevhead (91eb6695) serializeshttpBaseUrl, max message size, and timeout in the middleware-passthrough binding, but it does not serializeuseForwardedHost; repo search finds the option only inHttpOptions,HttpApiHandler, and host tests, with no corresponding host PR. Thus a real binding payload always takes this new default-off branch, so enablinghttpSettings.useForwardedHoststill cannot make Python 2.x honor either forwarded-header family. Please land/reference the host serialization change and test the exact emitted JSON.Also validate adopted values before building the origin: with a synthetic opt-in payload,
Forwarded: proto=;host=public.exampleorX-Forwarded-Proto: ,httpscurrently makes the check-statusLocationrelative (/custom/durable/instances/...?...code=...). Keep the request scheme/host for empty parsed values and havereplace_url_origin()reject an origin without both scheme and authority.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed both points. Commit
79f1ab5now preserves request components for empty forwarded values and rejects origins without a scheme and authority; the host serialization and rollout decision is tracked in #244 for @berndverst and @andystaples, so I am leaving this thread open pending that discussion.