Repository navigation
test: pin websiteUrl authority-parsing outcomes on untrusted profile data - #1661
Merged
Merged
Conversation
…data websiteUrl() turns the free-text "blog" field of a third-party GitHub profile into an href. Its safety against an authority bent without a surviving "@" comes from the WHATWG parser rather than from any explicit check, so a refactor can invert it silently: replacing the new URL() resolution in applyDefaultScheme() with a hand-rolled split on "@" makes good.example\@evil.example resolve to https://evil.example/ while every existing test in the file stays green. Pin the four unasserted outcomes, each with the reasoning for why it is the correct one: the backslash authority resolving to the leading visible host, a schemeless host:port dropped because the colon reads as a scheme (while a value carrying its own scheme keeps its port), a non-ASCII authority emitted as punycode, and a trailing NUL stripped where one inside the host is dropped. Tests only; no production behaviour changes. Closes #1660 Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com>
Closed
4 tasks done
Contributor
Author
|
Important Held for human review by the hive's ACMM level gate. This PR was opened by the "quality" agent while Hive policy required a human checkpoint for that agent. Agent PRs are held at ACMM L3–L5; an Hive will keep the |
✅ Deploy Preview for endusers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Test Improvement
Adds four characterization tests to
tests/profile-links.test.mjspinningauthority-parsing outcomes of
websiteUrl()(src/lib/profile-links.mjs) thatthe suite already produced but never asserted.
websiteUrl()turns the free-textblogfield of a third-party GitHub profileinto an
href:scripts/fetch-community-people.mjscopies it verbatim intodata/community-people.jsonand the scheduled refresh commits it unedited, sonothing reviews that string before it becomes a link. The existing tests cover
the disguise the module was written for — an explicit userinfo component — and
the protocols it refuses. They say nothing about an authority bent without a
surviving
@.The load-bearing one is the backslash case. A backslash is treated as a slash
inside a special scheme's authority, so
good.example\@evil.exampleresolvesto
https://good.example/@evil.example— the host is the label the reader seesfirst, and the userinfo guard never fires because
usernameis already emptyby the time it runs. That safety is a property of
new URL(), not of a checkin this repository.
Measured rather than asserted: replacing the
new URL()resolution inapplyDefaultScheme()with a hand-rolledvalue.split('@').pop()makes thatinput resolve to
https://evil.example/— the attacker's host wearing thevisible host's name — while every pre-existing test in the file stays
green. With this PR that mutation fails two tests. A second mutation,
narrowing
SCHEME_PATTERNto disallow dots, fails one.The other three pin a schemeless
host:portbeing dropped because the colonreads as a scheme (with the matching case showing a port is kept once the
value carries its own scheme, so the boundary is not mistaken for a port ban),
a non-ASCII authority emitting its punycode host, and a trailing NUL being
stripped where a NUL inside the host is dropped.
Each test carries the reasoning for why its outcome is the correct one, so a
future change to
applyDefaultScheme()orSCHEME_PATTERNhas to argue with ared test instead of passing silently.
Tests only — no production code changes, and no existing assertion altered.
Scope
Touches
tests/profile-links.test.mjsonly. Disjoint from the openhold-gated PRs: #1657 (
src/components/MetricsDashboard,formatDate) and#1659 (
src/components/shared/FilterToolbar, catalog filter UIs).Verification
node --test tests/profile-links.test.mjs— 20 pass, 0 failnpm run test:unit:coverage:check— exit 0 (the required "Validaterepository" gate)
npx prettier --check tests/profile-links.test.mjs— cleanThis is not a coverage gap, and is not filed as one:
src/lib/profile-links.mjsis 100% line-covered in both lanes — unit (100.00% source lines/regions) and
end-to-end (artifact
e2e-coverageid 12212515876, run39302402289-1,revision
e1f9b981b8a99d029888672940140b58ac759657: 100.00% lines, 86.67%regions, the residual regions being the two-build union-attribution artifact
already documented in
tests/e2e-coverage-gate.test.mjs). It is an assertiongap behind full coverage, which the coverage gates cannot see.
Related Issue
Closes #1660
Filed by quality agent (hold-gated mode). Human review required.
— hive: agent=quality backend=copilot model=claude-opus-5 copilot=1.0.88