sitetile: form coral gains thanks= for an owner-editable page after a successful inbox submit - #32
Merged
Merged
Conversation
… successful inbox submit
cverorg
force-pushed
the
feat/form-thanks-page
branch
from
September 9, 2026 14:05
ebc56b1 to
08464c5
Compare
…n still resolve off-site (#32 review round 1) A leading single-dot path segment or an ASCII control character is removed by a URL parser before it resolves a value, so an input that passed isSiteRelativePath/isSafeInternalPath on its raw, as-posted spelling could still resolve into a Location beginning with two slashes - a network-path reference a browser reads as a host, not a path. inboxRedirectResponse now checks the RESOLVED value instead: the origin must still be this site's, and the pathname must not start with //; either failing falls back to the site root. Applies to both return_to and thanks_to since they share the one function. Both isSiteRelativePath and isSafeInternalPath also strip ASCII tab/LF/CR up front, the same way isSafeImageSrc already does, so the two gates and the result-side check stay in step on that class. Form.astro's progressive-enhancement script no longer navigates to the forwarder's res.url unconditionally; it checks the resolved origin first and falls back to a full-page reload when it does not match. The no-JS path is covered by the forwarder's own result-side check. Added a property test (a reconstructed corpus of single-dot segments and their percent-encoded spellings, control characters, backslashes, and percent-encoded slashes) asserting that every Location this route can produce resolves on the site's own origin, plus the reviewer's pinned 13-shape set and the three known isSiteRelativePath/isSafeInternalPath entity-decoding divergences (safe direction). Also pins the return_to/thanks_to locale-rewrite asymmetry with a zh-tw fixture, softens two over-claiming sentences in the change's own write-up, and notes that takeDropWarnings has no consumer in this repo yet.
Member
Author
|
Round 1 fixes pushed (bdbc201): the forwarder now checks the resolved redirect target against the site origin and the pathname prefix and falls back when the check fails, for both fields that share the function; both gates strip control characters; the in-page script only navigates to a same-origin result; property-based tests over a generated corpus with non-vacuousness guards. Round 2 review in progress. |
Member
Author
|
Round 2 review: mergeable. Independent 592-input corpus through the emitted worker: 0 off-site after the fix (206 before), and the 206 changed Locations were exactly the off-site ones — safe values unchanged. The redirect base is a build-time constant, so the same-origin check is not a tautology and preview deploys keep working; failure outcomes return to the original page and only a tampered target falls back to the site root. One test added before merge so the forwarder's control-character stripping has its own ruler. |
…mutation-killing case (#32 review round 2) isSiteRelativePath's tab/LF/CR strip had no assertion that would fail if removed: the pinned- shape property loop continues past any value the gate rejects, and the corpus property only checks the final Location's origin/pathname, which the result-side check in inboxRedirectResponse already protects on its own regardless of this gate's own stripping. Added three end-to-end cases, one per control character, with it placed right after the leading slash of an otherwise ordinary thanks_to. That is the one position where stripping changes the gate's accept/reject decision: stripped, the value reveals a network-path reference and the gate rejects it before it reaches inboxRedirectResponse, so a successful submit lands on return_to. Without the strip, the gate admits the raw value and it is only caught later, by the result-side check on the origin a real URL parser resolves it to (dropping the control character itself) - whose own fallback is the site root, not return_to. Removing the strip changes which page a real visitor's successful submit lands on, and now goes red on exactly these three (100/103, matching the reviewer's count).
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
1 participant
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.
Site owners often want a page of their own after a form is submitted, to change the copy and add their own scripts. Today
form action=inboxreturns to the same page with?inbox=<outcome>and shows a built-in success card.thanks=<path>on the form coral. The value must be a path on this site (leading single slash, no scheme, no double slash, no..); anything else is dropped with a drop warning. A backslash is normalised like a slash before the check.thanks=set, the forwarder redirects a successful submit to that page with?inbox=sentappended (existing query preserved); failures still return to the original page so the inline retry message keeps working. The target travels as a hidden field and is re-validated by the forwarder itself.thanks=the built output is byte-identical except one new, otherwise dead branch in the shared inline script.Tests: parser accept/drop table, renderer with/without the attribute, forwarder Location logic, isolated hostile-fixture build; full
scripts/test.shgreen (187 / 70 / 166).