Repository navigation
feat: pass the status code and the reason to the framed error page - #137
Merged
Merged
Conversation
- Encode the context of the response onto the framed URL, keeping any query string the configured URL already carries and sorting the keys - Hand over the status phrase on the up-stream error, so the text of the back-end exception no longer reaches the page URL nor its title
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
joamag
marked this pull request as ready for review
November 6, 2026 11:02
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e281c28d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Move the context argument to the end of the page builders, so a call that passes the existing options by position keeps its behaviour - Place the parameters before any fragment, as they belong to the query
joamag
added a commit
that referenced
this pull request
Nov 6, 2026
) * feat: pass the status code and the reason to the framed error page - Encode the context of the response onto the framed URL, keeping any query string the configured URL already carries and sorting the keys - Hand over the status phrase on the up-stream error, so the text of the back-end exception no longer reaches the page URL nor its title * fix: keep the builder signatures and the fragment intact - Move the context argument to the end of the page builders, so a call that passes the existing options by position keeps its behaviour - Place the parameters before any fragment, as they belong to the query
joamag
added a commit
that referenced
this pull request
Nov 6, 2026
* feat: serve a configured page on the not authorized response - Add a not authorized URL map, settable per service by a consul tag, falling back to the error one so a single page may still cover both - Keep the status and the challenge header untouched, so the user agent still asks for the credentials and only the body is replaced * feat: pass the status code and the reason to the framed error page (#137) * feat: pass the status code and the reason to the framed error page - Encode the context of the response onto the framed URL, keeping any query string the configured URL already carries and sorting the keys - Hand over the status phrase on the up-stream error, so the text of the back-end exception no longer reaches the page URL nor its title * fix: keep the builder signatures and the fragment intact - Move the context argument to the end of the page builders, so a call that passes the existing options by position keeps its behaviour - Place the parameters before any fragment, as they belong to the query * docs: describe the error pages and correct the changelog - List the not found and the not authorized tags beside the error one, with the context parameters and the caveat of framing a page that the credentials of the request do not reach - Say what the not authorized entry actually changes, the plain body was the one answered before and the error page is now framed by default - Encode a quote in the framed URL, it would end the attribute early * fix: escape the ampersand of the framed URL - An ampersand opens a character reference in an attribute, so a query string carrying one was read as another URL by the browser * fix: keep the pages named in the environment across the consul polls - The cleanup of a rebuild dropped the entries of a host that consul manages, so a page named in the environment lasted until poll two - Applies them again before the tags, which still take precedence * docs: match the framed example and record the escaping - The example showed the raw ampersand, the attribute carries it escaped - Records that the pages already shipped have their URL framed as named
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #136.
Stacked on #135, which this needs so that all four responses already build their body through
build_data. Review that one first; the base will becomemasteronce it lands.What this does
build_dataandbuild_iframegain an optionalparamsargument, encoded onto the framed URL by a new_build_urlhelper, and each of the four responses passes its own code and reason:A page can now show the status code and the reason instead of being a fixed sheet of text, and a single configured page becomes viable again for anyone who prefers one over three, since it can branch on the code it is handed.
Details worth noting:
//cdn.hive.pt/down?v=2becomes?v=2&code=500&reason=...params, or nourl, nothing changes at allThe up-stream error does not hand over its message
The not found, the forbidden and the not authorized responses carry a fixed message, so forwarding it is harmless. The up-stream error does not: its message is the text of the back-end exception and can name internal addresses and ports.
While writing the tests I checked what actually reaches the visitor and found the message was still being put in front of them through the
<title>of the wrapper page, even with a page framed. That predates this work, it is not a regression from #132, but it sits squarely in what this issue is about, so it is fixed here: when a page is framed the response hands over the status phrase for both the reason and the title. With no page framed the message is still shown, since nothing else would report the cause.Two tests pin this, one asserting the internal address appears nowhere in the framed response, the other asserting it is still shown when nothing is framed.
Tests
test/servers/http.py- the parameters on the framed branch and their absence on the plain one,_build_urlwith no parameters, with an empty map, sorted, against a URL that already has a query string, against one ending in the separator, and the encoding of reserved characterstest/extra/proxy_r.py- the eight existing assertions now pin the full URL including the context, plus the two leak tests aboveThe eight updated assertions are the deliberate behaviour change of this PR, not a loosening: each now pins the exact URL rather than the bare one.
Verification
black --checkclean over all 345 filesmypy.stubtestreports no issues in 132 modules on Python 3.14, with the stub updated (Mappingadded to its imports)coverage run -m pytestand the repository.coveragercFollow-up, not in this PR
The pages in
hivesolutions/staticdo not read the parameters yet, they are static HTML. Reading them is a small piece of script on those pages, worth doing once this is released.