docs: say where the token goes when the origin pin is off #41

jercik · 2026-10-04T23:16:20Z

jercik commented

2026-10-04 23:16:20 +00:00

Follow-up to #37, from review 01M44HQNME6B9VAE2MGYCZKPXJ (summary-only finding 01M44J54BBK4B8V4BDJEWYP7XH).

The expected-service-origin description in action.yml said that with the pin off "only the https: scheme is enforced". That is wrong: reviewCredentials() always calls serviceOrigin(), which also rejects an unparseable URL and a path ending in /v1, pin or no pin. src/credentials.test.ts covers the /v1 rejection with the pin unset.

The description now says what an unset pin gives up: the capability token is sent to whatever https origin REVIEW_SERVICE_URL names. It doesn't list the other checks serviceOrigin() makes.

It targets main, because the sentence is older than #37. #34 removes the README's copy of this description.

🤖 Generated with Claude Code

Follow-up to #37, from review `01M44HQNME6B9VAE2MGYCZKPXJ` (summary-only finding `01M44J54BBK4B8V4BDJEWYP7XH`). The `expected-service-origin` description in `action.yml` said that with the pin off "only the `https:` scheme is enforced". That is wrong: `reviewCredentials()` always calls `serviceOrigin()`, which also rejects an unparseable URL and a path ending in `/v1`, pin or no pin. `src/credentials.test.ts` covers the `/v1` rejection with the pin unset. The description now says what an unset pin gives up: the capability token is sent to whatever https origin `REVIEW_SERVICE_URL` names. It doesn't list the other checks `serviceOrigin()` makes. It targets `main`, because the sentence is older than #37. #34 removes the README's copy of this description. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit

2026-10-04 23:16:20 +00:00

docs: say an unset origin pin still validates the service URL

commit-msg / commitlint (pull_request) Successful in 24s

Details

Checks / quality-checks (pull_request) Successful in 48s

Details

Review / Review (pull_request_target) Failing after 8m39s

Details

847c48bad8

The input description said that with the pin off only the https scheme
is enforced. reviewCredentials() always runs serviceOrigin(), which also
rejects a URL that is unparseable or whose path ends in /v1 or /v1/, pin
or no pin.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jercik referenced this pull request

2026-10-04 23:16:48 +00:00

docs: correct the service URL and PR-input rows of the environment table #37

forgejo-actions commented

2026-10-04 23:24:58 +00:00

Review 01M457CXZ067FRC2MQ7MR7PZTQ — head a68a0d7020f6db2e17ce3772a32e8e38c040a46f

Review — j4k-oss/review-wrapper @ `a0eb315dc5`

Scope: diff against base tree e69943819640
Status: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v4",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (1)

low — README's `expected-service-origin` row keeps the wording action.yml just replaced and misstates what is enforced when the pin is off

claim: 01M457F404F76GCPPQ924RPMQX
anchor: README.md (snippet)

the pin is off and only the `https:` scheme is enforced.

lens: general-bug · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M457GFJATTEH0DNYPW5AYHQJ · valid: The grounded README excerpt (exact match) says that with the pin off 'only the https: scheme is enforced.' The reviewer traces serviceOrigin() in src/credentials.ts as rejecting both a non-https protocol and a path matching //v1/?$/ whether or not the pin is set. That trace is specific and internally consistent. A prior claim on this same anchor reported a direct reviewCredentials() call with an empty pin and an https .../v1 URL that threw 'must not end with the /v1 API prefix', which supports it. So the README misstates what is enforced. The other claim in this batch quotes the new action.yml ending ('the capability token is sent to whatever https origin REVIEW_SERVICE_URL names'), which confirms that this diff, which touches only action.yml, left the README row stale. Nothing in the batch refutes this. The impact is misleading documentation, and a /v1 URL fails loudly at credential loading, so low severity fits.
duplicates: 01M457F7VH58P8H3HQESMP9P1G (writing-quality)
disposition: none

The README's Action inputs table copies the expected-service-origin description from action.yml. This change rewrote that description in action.yml but left the README copy alone. The two now disagree. Someone reading the README sees the unpinned case described as a scheme check. They are not told the consequence that action.yml now spells out: the capability token goes to any https origin that REVIEW_SERVICE_URL names.

The README row still ends "the pin is off and only the https: scheme is enforced." action.yml now reads "the pin is off, and the capability token is sent to whatever https origin REVIEW_SERVICE_URL names." The README sentence is also wrong as a statement of behavior. serviceOrigin() in src/credentials.ts enforces two things whether or not the pin is set: it rejects a non-https: protocol, and it rejects a path matching /\/v1\/?$/u ("REVIEW_SERVICE_URL must not end with the /v1 API prefix"). So the scheme is not the only check.

Fix: replace the README cell's last clause with the new action.yml wording, so the two copies match.

I read src/credentials.ts (serviceOrigin, reviewCredentials), the matching code in dist/index.mjs, the README's Action inputs table and Security model section, and the diff, which touches only action.yml. This is static reading; I ran nothing. To check it, compare the README table cell with the inputs.expected-service-origin.description in action.yml.

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (1)
- 01M457F7VH58P8H3HQESMP9P1G medium — The new expected-service-origin consequence is written only in action.yml; the README's copy of the same description still gives the old wording → 01M457F404F76GCPPQ924RPMQX
unadjudicated (0)

Coverage

Coverage pass: 01M457CY0RG0DWN4K9YXX5C18X
Accounting: complete
Slot health: healthy

lens	part	arm	unit status	runs	loss
general-bug	whole	default	claims-emitted	1	no
writing-quality	whole	default	claims-emitted	1	no
test-trimming	whole	default	no-claims	1	no
restated-sets	whole	default	no-claims	1	no
project-docs	whole	default	no-claims	1	no

**Review** `01M457CXZ067FRC2MQ7MR7PZTQ` — head `a68a0d7020f6db2e17ce3772a32e8e38c040a46f` # Review — j4k-oss/review-wrapper @ a0eb315dc549 Scope: diff against base tree `e69943819640` Status: dispatched — coverage complete (5/5 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v4", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (1) ### low — README's `expected-service-origin` row keeps the wording action.yml just replaced and misstates what is enforced when the pin is off - claim: `01M457F404F76GCPPQ924RPMQX` - anchor: `README.md` (snippet) ``` the pin is off and only the `https:` scheme is enforced. ``` - lens: general-bug · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M457GFJATTEH0DNYPW5AYHQJ` · valid: The grounded README excerpt (exact match) says that with the pin off 'only the https: scheme is enforced.' The reviewer traces serviceOrigin() in src/credentials.ts as rejecting both a non-https protocol and a path matching /\/v1\/?$/ whether or not the pin is set. That trace is specific and internally consistent. A prior claim on this same anchor reported a direct reviewCredentials() call with an empty pin and an https .../v1 URL that threw 'must not end with the /v1 API prefix', which supports it. So the README misstates what is enforced. The other claim in this batch quotes the new action.yml ending ('the capability token is sent to whatever https origin REVIEW_SERVICE_URL names'), which confirms that this diff, which touches only action.yml, left the README row stale. Nothing in the batch refutes this. The impact is misleading documentation, and a /v1 URL fails loudly at credential loading, so low severity fits. - duplicates: `01M457F7VH58P8H3HQESMP9P1G` (writing-quality) - disposition: none > The README's Action inputs table copies the `expected-service-origin` description from `action.yml`. This change rewrote that description in `action.yml` but left the README copy alone. The two now disagree. Someone reading the README sees the unpinned case described as a scheme check. They are not told the consequence that `action.yml` now spells out: the capability token goes to any https origin that `REVIEW_SERVICE_URL` names. > > The README row still ends "the pin is off and only the `https:` scheme is enforced." `action.yml` now reads "the pin is off, and the capability token is sent to whatever https origin REVIEW_SERVICE_URL names." The README sentence is also wrong as a statement of behavior. `serviceOrigin()` in `src/credentials.ts` enforces two things whether or not the pin is set: it rejects a non-`https:` protocol, and it rejects a path matching `/\/v1\/?$/u` ("REVIEW_SERVICE_URL must not end with the /v1 API prefix"). So the scheme is not the only check. > > Fix: replace the README cell's last clause with the new `action.yml` wording, so the two copies match. > > I read `src/credentials.ts` (`serviceOrigin`, `reviewCredentials`), the matching code in `dist/index.mjs`, the README's Action inputs table and Security model section, and the diff, which touches only `action.yml`. This is static reading; I ran nothing. To check it, compare the README table cell with the `inputs.expected-service-origin.description` in `action.yml`. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (1) - `01M457F7VH58P8H3HQESMP9P1G` medium — The new `expected-service-origin` consequence is written only in `action.yml`; the README's copy of the same description still gives the old wording → `01M457F404F76GCPPQ924RPMQX` - unadjudicated (0) ## Coverage Coverage pass: 01M457CY0RG0DWN4K9YXX5C18X Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | claims-emitted | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | no-claims | 1 | no | | restated-sets | whole | default | no-claims | 1 | no | | project-docs | whole | default | no-claims | 1 | no |

🚀 1

jercik commented

2026-10-04 23:40:22 +00:00

Replying to comment #124567

This is review 01M44K8JC1NST1842HWPYZG6DR of 847c48b. Neither pass has delivered all five slots: run 64348 lost two to model capacity, and dispatch run 64459 lost writing-quality, test-trimming and project-docs to sandbox infrastructure failures. I'll re-ask once the sandbox recovers.

The one finding so far, 01M44KDQDDN3TNYPPW9MY309TB (the README says an unset origin pin leaves only the https: check), is summary-only and valid. This PR corrects the description in action.yml; the README row is a copy of it. #34 replaces that README table with a pointer to inputs in action.yml, so the copy goes away. The duplicate claim 01M44M5SHRX77G73JD4EFS6HD8 is covered by this finding.

> Replying to comment #124567 This is review `01M44K8JC1NST1842HWPYZG6DR` of `847c48b`. Neither pass has delivered all five slots: run 64348 lost two to model capacity, and dispatch run 64459 lost writing-quality, test-trimming and project-docs to sandbox infrastructure failures. I'll re-ask once the sandbox recovers. The one finding so far, `01M44KDQDDN3TNYPPW9MY309TB` (the README says an unset origin pin leaves only the `https:` check), is summary-only and valid. This PR corrects the description in `action.yml`; the README row is a copy of it. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/34 replaces that README table with a pointer to `inputs` in `action.yml`, so the copy goes away. The duplicate claim `01M44M5SHRX77G73JD4EFS6HD8` is covered by this finding.

forgejo-actions reviewed

2026-10-05 00:18:08 +00:00

action.yml Outdated

Lines 11-12

					
				@ -9,3 +9,4 @@

				      Origin REVIEW_SERVICE_URL must match exactly, e.g. https://review.j4k.dev.

				      Left unset — or set to the empty string an undefined vars expression

				      expands to — the pin is off and only the https scheme is enforced.

				      expands to — the pin is off: the origin is not compared, and

				      REVIEW_SERVICE_URL is still validated.

medium — expected-service-origin description replaces the security consequence of leaving the pin off with an unspecified "is still validated"

A workflow author reading this input's description to decide whether to set it learns that the origin is not compared and that REVIEW_SERVICE_URL "is still validated", which reads as if some other check still protects the token. No check does: with the pin off, the capability token goes to whatever https: URL the org variable names. The previous wording ("only the https scheme is enforced") at least signalled that nothing restricts the host; the new clause drops that signal without saying what "validated" covers.

The code states the stake directly. reviewCredentials in src/credentials.ts comments: "The capability token rides an Authorization header to whatever this variable names, so a pinned origin is the only thing that stops a repointed org variable from posting it somewhere else." serviceOrigin only checks that the value parses as a URL, uses https:, and does not end in /v1; none of those constrain the destination host.

Correction: replace the last clause with the consequence instead of a vague assurance, and leave the format checks to their existing home:

Left unset — or set to the empty string an undefined vars expression expands to — the pin is off, and the capability token is sent to whatever origin REVIEW_SERVICE_URL names.

This keeps the useful meaning (unset means no origin comparison) and tells the reader what they give up, which is the decision the description exists to inform. It names no validation rules, so it cannot go stale when serviceOrigin changes; README's environment-contract row already documents the URL format checks.

Examined: the diff to action.yml, serviceOrigin and reviewCredentials in src/credentials.ts, and the README's Security model section, which also says pinning is what stops a repointed variable from posting the token elsewhere. Whether workflow authors actually misread "still validated" as host protection is a reader-behaviour judgment, not something I tested.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M44PKWXVGGJX4FMQ5FKXPKX9 of review 01M44K8JC1NST1842HWPYZG6DR

**medium** — `expected-service-origin` description replaces the security consequence of leaving the pin off with an unspecified "is still validated" > A workflow author reading this input's description to decide whether to set it learns that the origin is not compared and that REVIEW_SERVICE_URL "is still validated", which reads as if some other check still protects the token. No check does: with the pin off, the capability token goes to whatever `https:` URL the org variable names. The previous wording ("only the https scheme is enforced") at least signalled that nothing restricts the host; the new clause drops that signal without saying what "validated" covers. > > The code states the stake directly. `reviewCredentials` in `src/credentials.ts` comments: "The capability token rides an Authorization header to whatever this variable names, so a pinned origin is the only thing that stops a repointed org variable from posting it somewhere else." `serviceOrigin` only checks that the value parses as a URL, uses `https:`, and does not end in `/v1`; none of those constrain the destination host. > > Correction: replace the last clause with the consequence instead of a vague assurance, and leave the format checks to their existing home: > > > Left unset — or set to the empty string an undefined vars expression expands to — the pin is off, and the capability token is sent to whatever origin REVIEW_SERVICE_URL names. > > This keeps the useful meaning (unset means no origin comparison) and tells the reader what they give up, which is the decision the description exists to inform. It names no validation rules, so it cannot go stale when `serviceOrigin` changes; README's environment-contract row already documents the URL format checks. > > Examined: the diff to `action.yml`, `serviceOrigin` and `reviewCredentials` in `src/credentials.ts`, and the README's Security model section, which also says pinning is what stops a repointed variable from posting the token elsewhere. Whether workflow authors actually misread "still validated" as host protection is a reader-behaviour judgment, not something I tested. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44PKWXVGGJX4FMQ5FKXPKX9` of review `01M44K8JC1NST1842HWPYZG6DR`

👍 1

Fixed in 4b4bcbc. The description now ends "the pin is off, and the capability token is sent to whatever origin REVIEW_SERVICE_URL names", as proposed. It makes no claim about which checks remain, so it can't drift from serviceOrigin. I retitled the PR to match.

Fixed in 4b4bcbc. The description now ends "the pin is off, and the capability token is sent to whatever origin REVIEW_SERVICE_URL names", as proposed. It makes no claim about which checks remain, so it can't drift from `serviceOrigin`. I retitled the PR to match.

jercik marked this conversation as resolved

jercik referenced this pull request

2026-10-05 00:18:17 +00:00

docs: correct the service URL and PR-input rows of the environment table #37

jercik added 1 commit

2026-10-05 00:23:25 +00:00

docs: say where the token goes when the origin pin is off

commit-msg / commitlint (pull_request) Successful in 22s

Details

Checks / quality-checks (pull_request) Successful in 33s

Details

Review / Review (pull_request_target) Successful in 2m5s

Details

4b4bcbc270

"REVIEW_SERVICE_URL is still validated" named no check and read as if something still protected
the token. Nothing does: `serviceOrigin` checks the URL's form, not its host. The description now
states what leaving the input unset gives up.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jercik changed title from ~~docs: say an unset origin pin still validates the service URL~~ to docs: say where the token goes when the origin pin is off

2026-10-05 00:23:38 +00:00

jercik commented

2026-10-05 00:23:41 +00:00

Replying to comment #124567

Dispatch run 64849 completed review 01M44K8JC1NST1842HWPYZG6DR of 847c48b with all five slots. The finding on action.yml has its own thread and is fixed in 4b4bcbc.

01M44KDQDDN3TNYPPW9MY309TB (the README row still says only the https: scheme is enforced) stands as answered above: #34 replaces that README table with a pointer to inputs in action.yml. The four duplicate claims are covered by it.

> Replying to comment #124567 Dispatch run 64849 completed review `01M44K8JC1NST1842HWPYZG6DR` of `847c48b` with all five slots. The finding on `action.yml` has its own thread and is fixed in 4b4bcbc. `01M44KDQDDN3TNYPPW9MY309TB` (the README row still says only the `https:` scheme is enforced) stands as answered above: https://code.j4k.dev/j4k-oss/review-wrapper/pulls/34 replaces that README table with a pointer to `inputs` in `action.yml`. The four duplicate claims are covered by it.

forgejo-actions reviewed

2026-10-05 00:25:43 +00:00

action.yml Outdated

Lines 11-12

					
				@ -9,3 +9,4 @@

				      Origin REVIEW_SERVICE_URL must match exactly, e.g. https://review.j4k.dev.

				      Left unset — or set to the empty string an undefined vars expression

				      expands to — the pin is off and only the https scheme is enforced.

				      expands to — the pin is off, and the capability token is sent to

				      whatever origin REVIEW_SERVICE_URL names.

low — expected-service-origin description says the token goes to whatever origin REVIEW_SERVICE_URL names, but non-https origins are rejected first

The rewritten input description drops the clause "only the https scheme is enforced" and replaces it with a claim that, with the pin off, the capability token is sent to "whatever origin REVIEW_SERVICE_URL names". That is false for any non-https: origin. A workflow author reading the action metadata is told that leaving the pin off gives no transport guarantee at all, and is no longer told that an http: URL fails the run.

In src/credentials.ts, reviewCredentials() calls serviceOrigin(serviceUrl) before it reads the token with requireEnv("REVIEW_CAPABILITY_TOKEN"). serviceOrigin throws REVIEW_SERVICE_URL must use https, got ${parsed.protocol} in ${serviceUrl} when parsed.protocol !== "https:", and it also rejects a URL whose path ends in /v1. The bundled dist/index.mjs contains the same check. So with the pin off, the token reaches only https: origins, never every origin.

The two documents now disagree. README.md's "Action inputs" table still describes the same input as "the pin is off and only the https: scheme is enforced", and its Security model section says the variable "is parsed as an https: URL before the token is read".

Correction: keep both facts in action.yml, for example "the pin is off: only the https scheme is enforced, and the capability token is sent to any https origin REVIEW_SERVICE_URL names." Then make the README row match.

This comes from reading the code, not from running it. I read serviceOrigin and reviewCredentials in src/credentials.ts, grepped dist/index.mjs for the https check, and compared both against README.md. The claim is wrong only if some other code path sends the token without calling reviewCredentials(). The README says src/credentials.ts is the only module that reads the token, and the grep found nothing else that reads it.

lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M44Q4MW5M5F02YR6MA37DYH7 of review 01M44Q3C0VKVB6T4BS7N5R5133

**low** — `expected-service-origin` description says the token goes to whatever origin REVIEW_SERVICE_URL names, but non-https origins are rejected first > The rewritten input description drops the clause "only the https scheme is enforced" and replaces it with a claim that, with the pin off, the capability token is sent to "whatever origin REVIEW_SERVICE_URL names". That is false for any non-`https:` origin. A workflow author reading the action metadata is told that leaving the pin off gives no transport guarantee at all, and is no longer told that an `http:` URL fails the run. > > In `src/credentials.ts`, `reviewCredentials()` calls `serviceOrigin(serviceUrl)` before it reads the token with `requireEnv("REVIEW_CAPABILITY_TOKEN")`. `serviceOrigin` throws `REVIEW_SERVICE_URL must use https, got ${parsed.protocol} in ${serviceUrl}` when `parsed.protocol !== "https:"`, and it also rejects a URL whose path ends in `/v1`. The bundled `dist/index.mjs` contains the same check. So with the pin off, the token reaches only `https:` origins, never every origin. > > The two documents now disagree. README.md's "Action inputs" table still describes the same input as "the pin is off and only the `https:` scheme is enforced", and its Security model section says the variable "is parsed as an `https:` URL before the token is read". > > Correction: keep both facts in action.yml, for example "the pin is off: only the https scheme is enforced, and the capability token is sent to any https origin REVIEW_SERVICE_URL names." Then make the README row match. > > This comes from reading the code, not from running it. I read `serviceOrigin` and `reviewCredentials` in src/credentials.ts, grepped dist/index.mjs for the https check, and compared both against README.md. The claim is wrong only if some other code path sends the token without calling `reviewCredentials()`. The README says src/credentials.ts is the only module that reads the token, and the grep found nothing else that reads it. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44Q4MW5M5F02YR6MA37DYH7` of review `01M44Q3C0VKVB6T4BS7N5R5133`

👍 1

low — expected-service-origin description says the token goes to whatever origin REVIEW_SERVICE_URL names, but non-https origins are rejected first

The rewritten input description drops the clause "only the https scheme is enforced" and replaces it with a claim that, with the pin off, the capability token is sent to "whatever origin REVIEW_SERVICE_URL names". That is false for any non-https: origin. A workflow author reading the action metadata is told that leaving the pin off gives no transport guarantee at all, and is no longer told that an http: URL fails the run.

In src/credentials.ts, reviewCredentials() calls serviceOrigin(serviceUrl) before it reads the token with requireEnv("REVIEW_CAPABILITY_TOKEN"). serviceOrigin throws REVIEW_SERVICE_URL must use https, got ${parsed.protocol} in ${serviceUrl} when parsed.protocol !== "https:", and it also rejects a URL whose path ends in /v1. The bundled dist/index.mjs contains the same check. So with the pin off, the token reaches only https: origins, never every origin.

The two documents now disagree. README.md's "Action inputs" table still describes the same input as "the pin is off and only the https: scheme is enforced", and its Security model section says the variable "is parsed as an https: URL before the token is read".

Correction: keep both facts in action.yml, for example "the pin is off: only the https scheme is enforced, and the capability token is sent to any https origin REVIEW_SERVICE_URL names." Then make the README row match.

This comes from reading the code, not from running it. I read serviceOrigin and reviewCredentials in src/credentials.ts, grepped dist/index.mjs for the https check, and compared both against README.md. The claim is wrong only if some other code path sends the token without calling reviewCredentials(). The README says src/credentials.ts is the only module that reads the token, and the grep found nothing else that reads it.

lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M44Q4MW5M5F02YR6MA37DYH7 of review 01M44Q3C0VKVB6T4BS7N5R5133

**low** — `expected-service-origin` description says the token goes to whatever origin REVIEW_SERVICE_URL names, but non-https origins are rejected first > The rewritten input description drops the clause "only the https scheme is enforced" and replaces it with a claim that, with the pin off, the capability token is sent to "whatever origin REVIEW_SERVICE_URL names". That is false for any non-`https:` origin. A workflow author reading the action metadata is told that leaving the pin off gives no transport guarantee at all, and is no longer told that an `http:` URL fails the run. > > In `src/credentials.ts`, `reviewCredentials()` calls `serviceOrigin(serviceUrl)` before it reads the token with `requireEnv("REVIEW_CAPABILITY_TOKEN")`. `serviceOrigin` throws `REVIEW_SERVICE_URL must use https, got ${parsed.protocol} in ${serviceUrl}` when `parsed.protocol !== "https:"`, and it also rejects a URL whose path ends in `/v1`. The bundled `dist/index.mjs` contains the same check. So with the pin off, the token reaches only `https:` origins, never every origin. > > The two documents now disagree. README.md's "Action inputs" table still describes the same input as "the pin is off and only the `https:` scheme is enforced", and its Security model section says the variable "is parsed as an `https:` URL before the token is read". > > Correction: keep both facts in action.yml, for example "the pin is off: only the https scheme is enforced, and the capability token is sent to any https origin REVIEW_SERVICE_URL names." Then make the README row match. > > This comes from reading the code, not from running it. I read `serviceOrigin` and `reviewCredentials` in src/credentials.ts, grepped dist/index.mjs for the https check, and compared both against README.md. The claim is wrong only if some other code path sends the token without calling `reviewCredentials()`. The README says src/credentials.ts is the only module that reads the token, and the grep found nothing else that reads it. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44Q4MW5M5F02YR6MA37DYH7` of review `01M44Q3C0VKVB6T4BS7N5R5133`

👍 1

Fixed in a68a0d7. The description now ends "the capability token is sent to whatever https origin REVIEW_SERVICE_URL names". With the pin off, serviceOrigin() still rejects a URL that is not https: before the token is read, so "whatever origin" promised the token to an http: URL it never reaches. The PR body says the same.

Fixed in a68a0d7. The description now ends "the capability token is sent to whatever https origin REVIEW_SERVICE_URL names". With the pin off, `serviceOrigin()` still rejects a URL that is not `https:` before the token is read, so "whatever origin" promised the token to an `http:` URL it never reaches. The PR body says the same.

superseded by review 01M457CXZ067FRC2MQ7MR7PZTQ for head a68a0d7020f6db2e17ce3772a32e8e38c040a46f

superseded by review `01M457CXZ067FRC2MQ7MR7PZTQ` for head `a68a0d7020f6db2e17ce3772a32e8e38c040a46f`

🚀 1

Fixed in a68a0d7. This is a second copy of the claim in thread #125567, posted by the same review at the same moment; the answer there covers it.

Fixed in a68a0d7. This is a second copy of the claim in thread #125567, posted by the same review at the same moment; the answer there covers it.

jercik marked this conversation as resolved

forgejo-actions reviewed

2026-10-05 00:25:43 +00:00

action.yml Outdated

Lines 11-12

					
				@ -9,3 +9,4 @@

				      Origin REVIEW_SERVICE_URL must match exactly, e.g. https://review.j4k.dev.

				      Left unset — or set to the empty string an undefined vars expression

				      expands to — the pin is off and only the https scheme is enforced.

				      expands to — the pin is off, and the capability token is sent to

				      whatever origin REVIEW_SERVICE_URL names.

low — expected-service-origin description says the token goes to whatever origin REVIEW_SERVICE_URL names, but non-https origins are rejected first

The rewritten input description drops the clause "only the https scheme is enforced" and replaces it with a claim that, with the pin off, the capability token is sent to "whatever origin REVIEW_SERVICE_URL names". That is false for any non-https: origin. A workflow author reading the action metadata is told that leaving the pin off gives no transport guarantee at all, and is no longer told that an http: URL fails the run.

In src/credentials.ts, reviewCredentials() calls serviceOrigin(serviceUrl) before it reads the token with requireEnv("REVIEW_CAPABILITY_TOKEN"). serviceOrigin throws REVIEW_SERVICE_URL must use https, got ${parsed.protocol} in ${serviceUrl} when parsed.protocol !== "https:", and it also rejects a URL whose path ends in /v1. The bundled dist/index.mjs contains the same check. So with the pin off, the token reaches only https: origins, never every origin.

The two documents now disagree. README.md's "Action inputs" table still describes the same input as "the pin is off and only the https: scheme is enforced", and its Security model section says the variable "is parsed as an https: URL before the token is read".

Correction: keep both facts in action.yml, for example "the pin is off: only the https scheme is enforced, and the capability token is sent to any https origin REVIEW_SERVICE_URL names." Then make the README row match.

This comes from reading the code, not from running it. I read serviceOrigin and reviewCredentials in src/credentials.ts, grepped dist/index.mjs for the https check, and compared both against README.md. The claim is wrong only if some other code path sends the token without calling reviewCredentials(). The README says src/credentials.ts is the only module that reads the token, and the grep found nothing else that reads it.

lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M44Q4MW5M5F02YR6MA37DYH7 of review 01M44Q3C0VKVB6T4BS7N5R5133

**low** — `expected-service-origin` description says the token goes to whatever origin REVIEW_SERVICE_URL names, but non-https origins are rejected first > The rewritten input description drops the clause "only the https scheme is enforced" and replaces it with a claim that, with the pin off, the capability token is sent to "whatever origin REVIEW_SERVICE_URL names". That is false for any non-`https:` origin. A workflow author reading the action metadata is told that leaving the pin off gives no transport guarantee at all, and is no longer told that an `http:` URL fails the run. > > In `src/credentials.ts`, `reviewCredentials()` calls `serviceOrigin(serviceUrl)` before it reads the token with `requireEnv("REVIEW_CAPABILITY_TOKEN")`. `serviceOrigin` throws `REVIEW_SERVICE_URL must use https, got ${parsed.protocol} in ${serviceUrl}` when `parsed.protocol !== "https:"`, and it also rejects a URL whose path ends in `/v1`. The bundled `dist/index.mjs` contains the same check. So with the pin off, the token reaches only `https:` origins, never every origin. > > The two documents now disagree. README.md's "Action inputs" table still describes the same input as "the pin is off and only the `https:` scheme is enforced", and its Security model section says the variable "is parsed as an `https:` URL before the token is read". > > Correction: keep both facts in action.yml, for example "the pin is off: only the https scheme is enforced, and the capability token is sent to any https origin REVIEW_SERVICE_URL names." Then make the README row match. > > This comes from reading the code, not from running it. I read `serviceOrigin` and `reviewCredentials` in src/credentials.ts, grepped dist/index.mjs for the https check, and compared both against README.md. The claim is wrong only if some other code path sends the token without calling `reviewCredentials()`. The README says src/credentials.ts is the only module that reads the token, and the grep found nothing else that reads it. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44Q4MW5M5F02YR6MA37DYH7` of review `01M44Q3C0VKVB6T4BS7N5R5133`

👍 1

low — expected-service-origin description says the token goes to whatever origin REVIEW_SERVICE_URL names, but non-https origins are rejected first

The rewritten input description drops the clause "only the https scheme is enforced" and replaces it with a claim that, with the pin off, the capability token is sent to "whatever origin REVIEW_SERVICE_URL names". That is false for any non-https: origin. A workflow author reading the action metadata is told that leaving the pin off gives no transport guarantee at all, and is no longer told that an http: URL fails the run.

In src/credentials.ts, reviewCredentials() calls serviceOrigin(serviceUrl) before it reads the token with requireEnv("REVIEW_CAPABILITY_TOKEN"). serviceOrigin throws REVIEW_SERVICE_URL must use https, got ${parsed.protocol} in ${serviceUrl} when parsed.protocol !== "https:", and it also rejects a URL whose path ends in /v1. The bundled dist/index.mjs contains the same check. So with the pin off, the token reaches only https: origins, never every origin.

The two documents now disagree. README.md's "Action inputs" table still describes the same input as "the pin is off and only the https: scheme is enforced", and its Security model section says the variable "is parsed as an https: URL before the token is read".

Correction: keep both facts in action.yml, for example "the pin is off: only the https scheme is enforced, and the capability token is sent to any https origin REVIEW_SERVICE_URL names." Then make the README row match.

This comes from reading the code, not from running it. I read serviceOrigin and reviewCredentials in src/credentials.ts, grepped dist/index.mjs for the https check, and compared both against README.md. The claim is wrong only if some other code path sends the token without calling reviewCredentials(). The README says src/credentials.ts is the only module that reads the token, and the grep found nothing else that reads it.

lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M44Q4MW5M5F02YR6MA37DYH7 of review 01M44Q3C0VKVB6T4BS7N5R5133

**low** — `expected-service-origin` description says the token goes to whatever origin REVIEW_SERVICE_URL names, but non-https origins are rejected first > The rewritten input description drops the clause "only the https scheme is enforced" and replaces it with a claim that, with the pin off, the capability token is sent to "whatever origin REVIEW_SERVICE_URL names". That is false for any non-`https:` origin. A workflow author reading the action metadata is told that leaving the pin off gives no transport guarantee at all, and is no longer told that an `http:` URL fails the run. > > In `src/credentials.ts`, `reviewCredentials()` calls `serviceOrigin(serviceUrl)` before it reads the token with `requireEnv("REVIEW_CAPABILITY_TOKEN")`. `serviceOrigin` throws `REVIEW_SERVICE_URL must use https, got ${parsed.protocol} in ${serviceUrl}` when `parsed.protocol !== "https:"`, and it also rejects a URL whose path ends in `/v1`. The bundled `dist/index.mjs` contains the same check. So with the pin off, the token reaches only `https:` origins, never every origin. > > The two documents now disagree. README.md's "Action inputs" table still describes the same input as "the pin is off and only the `https:` scheme is enforced", and its Security model section says the variable "is parsed as an `https:` URL before the token is read". > > Correction: keep both facts in action.yml, for example "the pin is off: only the https scheme is enforced, and the capability token is sent to any https origin REVIEW_SERVICE_URL names." Then make the README row match. > > This comes from reading the code, not from running it. I read `serviceOrigin` and `reviewCredentials` in src/credentials.ts, grepped dist/index.mjs for the https check, and compared both against README.md. The claim is wrong only if some other code path sends the token without calling `reviewCredentials()`. The README says src/credentials.ts is the only module that reads the token, and the grep found nothing else that reads it. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44Q4MW5M5F02YR6MA37DYH7` of review `01M44Q3C0VKVB6T4BS7N5R5133`

👍 1

Fixed in a68a0d7. The description now ends "the capability token is sent to whatever https origin REVIEW_SERVICE_URL names". With the pin off, serviceOrigin() still rejects a URL that is not https: before the token is read, so "whatever origin" promised the token to an http: URL it never reaches. The PR body says the same.

Fixed in a68a0d7. The description now ends "the capability token is sent to whatever https origin REVIEW_SERVICE_URL names". With the pin off, `serviceOrigin()` still rejects a URL that is not `https:` before the token is read, so "whatever origin" promised the token to an `http:` URL it never reaches. The PR body says the same.

superseded by review 01M457CXZ067FRC2MQ7MR7PZTQ for head a68a0d7020f6db2e17ce3772a32e8e38c040a46f

superseded by review `01M457CXZ067FRC2MQ7MR7PZTQ` for head `a68a0d7020f6db2e17ce3772a32e8e38c040a46f`

🚀 1

Fixed in a68a0d7. This is a second copy of the claim in thread #125567, posted by the same review at the same moment; the answer there covers it.

Fixed in a68a0d7. This is a second copy of the claim in thread #125567, posted by the same review at the same moment; the answer there covers it.

jercik marked this conversation as resolved

jercik added 1 commit

2026-10-05 05:08:15 +00:00

docs: say an unpinned token still goes only to an https origin

commit-msg / commitlint (pull_request) Successful in 27s

Details

Checks / quality-checks (pull_request) Successful in 59s

Details

Review / Review (pull_request_target) Successful in 2m35s

Details

a68a0d7020

With the pin off, `serviceOrigin` rejects any URL that is not `https:` before the token is
read. "Whatever origin" told a workflow author that an `http:` URL would receive the token.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jercik commented

2026-10-05 05:08:41 +00:00

Replying to comment #124567

Round 2, review 01M44Q3C0VKVB6T4BS7N5R5133 of 4b4bcbc; run 64911 delivered all five slots. Its finding 01M44Q4MW5M5F02YR6MA37DYH7 is fixed in a68a0d7 and answered in thread #125567. The duplicate 01M44Q58J73J0RFY40J9KQGC6W is covered by that fix.

> Replying to comment #124567 Round 2, review `01M44Q3C0VKVB6T4BS7N5R5133` of `4b4bcbc`; run 64911 delivered all five slots. Its finding `01M44Q4MW5M5F02YR6MA37DYH7` is fixed in a68a0d7 and answered in thread #125567. The duplicate `01M44Q58J73J0RFY40J9KQGC6W` is covered by that fix.

jercik commented

2026-10-05 05:26:16 +00:00

Replying to comment #124567

Round 3, review 01M457CXZ067FRC2MQ7MR7PZTQ of a68a0d7, all five slots delivered. Its one finding, 01M457F404F76GCPPQ924RPMQX (the README's expected-service-origin row still says only the https: scheme is enforced), is summary-only and valid, and it is the claim round 1 raised as 01M44KDQDDN3TNYPPW9MY309TB. #34 still replaces that README table with a pointer to inputs in action.yml, so the stale copy goes away there. The duplicate 01M457F7VH58P8H3HQESMP9P1G is covered by it.

Thread #125568 was a second copy of the round-2 claim answered in #125567; it is answered the same way.

> Replying to comment #124567 Round 3, review `01M457CXZ067FRC2MQ7MR7PZTQ` of `a68a0d7`, all five slots delivered. Its one finding, `01M457F404F76GCPPQ924RPMQX` (the README's `expected-service-origin` row still says only the `https:` scheme is enforced), is summary-only and valid, and it is the claim round 1 raised as `01M44KDQDDN3TNYPPW9MY309TB`. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/34 still replaces that README table with a pointer to `inputs` in `action.yml`, so the stale copy goes away there. The duplicate `01M457F7VH58P8H3HQESMP9P1G` is covered by it. Thread #125568 was a second copy of the round-2 claim answered in #125567; it is answered the same way.

jercik merged commit 2e9a0335e6 into main

2026-10-05 09:34:50 +00:00

jercik referenced this pull request from a commit

2026-10-05 09:34:52 +00:00

docs: say where the token goes when the origin pin is off (#41)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

j4k-oss/review-wrapper!41

Rows
Columns

docs: say where the token goes when the origin pin is off #41

Review — j4k-oss/review-wrapper @ a0eb315dc5

Findings (1)

low — README's expected-service-origin row keeps the wording action.yml just replaced and misstates what is enforced when the pin is off

Other claims

Coverage

Review — j4k-oss/review-wrapper @ `a0eb315dc5`

low — README's `expected-service-origin` row keeps the wording action.yml just replaced and misstates what is enforced when the pin is off