chore: forward-port the v1.6.0 GA work to v1.7 - #702
Conversation
The governance model, security assurance case, and Best Practices badge landed on dev/v1.6 in #699 and reached main with the v1.6.0 GA sync, which is what earned the Silver badge. None of it was on dev/v1.7, so the first v1.7 sync to main would have deleted the badge from the front page and dropped documentation_achievements back to Unmet. Cherry-picked from a8b3367 (#699).
The REST /stargazers endpoint now 401s anonymously, and for a fine-grained token it demands contents=write — far more than a public star chart should hold. The route fell back on every request, so getdrydock.com just said "loading". The GraphQL stargazers connection needs only metadata=read and returns starredAt directly. Fixed on dev/v1.6 and verified live in production against the v1.6.0 GA deploy; v1.7 still had the broken REST path. Cherry-picked from 3881965.
The 7-day soak floor (604800s) is untouched. A workflow_dispatch soak_override_reason input hard-fails when blank or under 20 characters, emits a ::warning::, and stamps the justification into the job summary and release notes. The reason is passed through env and a temp file, never interpolated into a workflow command, so a multiline reason cannot smuggle a second :: directive. Used once, for the v1.6.0 GA cut at 3.0 days. v1.7 should have the same escape hatch and the same audit trail. Cherry-picked from 2b55ff4.
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe release workflow adds a GA-only Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/tests/release-cut-soak-override.test.ts (1)
229-238: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the 20-character boundary case.
The suite covers empty, whitespace-only, and 1 character, then jumps to a 68-character reason. The gate is
[ "${#trimmed_reason}" -lt 20 ]. An off-by-one flip to-lewould keep every existing test green.♻️ Pin both sides of the boundary
+test('a soak_override_reason of exactly 20 characters after trimming is accepted', () => { + const reason = 'x'.repeat(20); + const result = runSourceStep({ + isPrerelease: false, + ageSeconds: THREE_DAYS, + soakOverrideReason: ` ${reason} `, + }); + + expect(result.status).toBe(0); + expect(result.output.soak_override_used).toBe('true'); + expect(result.reasonFileContent).toBe(reason); +}); + +test('a soak_override_reason of 19 characters after trimming is rejected', () => { + const result = runSourceStep({ + isPrerelease: false, + ageSeconds: THREE_DAYS, + soakOverrideReason: 'x'.repeat(19), + }); + + expect(result.status).not.toBe(0); + expect(result.stdout).toContain('soak_override_reason must be at least 20 characters'); +});The 20-character case also asserts that trimming is applied before the length check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/tests/release-cut-soak-override.test.ts around lines 229 - 238, Add a test alongside the existing soak_override_reason validation cases that uses a reason whose trimmed length is exactly 20 characters, including surrounding whitespace, and assert the release step succeeds. This should pin the inclusive boundary and verify trimming occurs before the length check in runSourceStep..github/tests/release-cut-retry-workflow.test.ts (1)
227-240: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the injection guard to
with.commandbodies.The filter at Line 229 matches any step that mentions
soak_override_reason, but the assertion at Line 239 only inspectsstep.run.release-cut.ymlruns shell throughnick-fields/retryviawith.commandin several steps. If a future step passes the reason to auses:-based action with an inline command body, this test still passes while the template injection ships.♻️ Cover both shell surfaces
for (const step of stepsReferencingReason) { // The raw input must only ever appear on the right-hand side of an `env:` // mapping (e.g. `SOAK_OVERRIDE_REASON: ${{ inputs.soak_override_reason }}`), // never templated straight into `run:`, which is how a candidate_tag-style // template-injection finding would happen. expect(step.run ?? '').not.toContain('inputs.soak_override_reason'); + // Same rule for action-supplied shell bodies (e.g. nick-fields/retry). + expect(String(step.with?.command ?? '')).not.toContain('inputs.soak_override_reason'); }Confirm that the
WorkflowSteptype exposeswith; widen it if not.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/tests/release-cut-retry-workflow.test.ts around lines 227 - 240, Extend the injection assertion in the release-step test to inspect both `step.run` and the `with.command` shell body for direct `inputs.soak_override_reason` interpolation. Confirm the `WorkflowStep` type exposes the `with` field and widen that type if necessary, while preserving the existing filtering and assertion behavior..github/workflows/release-cut.yml (1)
1136-1150: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a multiline reason and a missing reason file.
Two gaps in this block:
cat "${SOAK_OVERRIDE_REASON_PATH}"runs underset -euo pipefail. Ifsoak_override_usedistruebut the file is absent, the step aborts after the tag and images are already prepared. The path is also unquoted-safe but unvalidated.- The source step accepts a multiline reason (only leading/trailing whitespace is trimmed).
echo "- Reason: ${override_reason}"then emits the second and later lines as top-level markdown, which breaks the bullet list and can inject headings into the published release body.♻️ Fix both
if [ "${SOAK_OVERRIDE_USED:-false}" = "true" ]; then - override_reason="$(cat "${SOAK_OVERRIDE_REASON_PATH}")" + if [ ! -f "${SOAK_OVERRIDE_REASON_PATH:-}" ]; then + echo "::error::soak_override_used=true but the recorded reason file '${SOAK_OVERRIDE_REASON_PATH:-}' is missing; refusing to publish an unjustified shortened soak." + exit 1 + fi { echo "" echo "---" echo "" echo "**Note:** the standard seven-day release-candidate soak was shortened for this release." echo "- Candidate age at promotion: ${SOAK_OVERRIDE_AGE_SECONDS}s (~${SOAK_OVERRIDE_AGE_DAYS} days), short of the usual 7 days (604800s)." - echo "- Reason: ${override_reason}" + echo "- Reason:" + # Indent every line so a multiline reason stays inside the list + # item instead of becoming top-level markdown. + sed -e 's/^/ /' "${SOAK_OVERRIDE_REASON_PATH}" } >> "${notes_path}" fiThe same multiline concern applies to the job-summary block at Lines 313-318.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-cut.yml around lines 1136 - 1150, Harden the soak-override reason handling in the release-body and job-summary blocks: validate that SOAK_OVERRIDE_REASON_PATH is set and points to a readable file before reading it, and fall back to a safe indication when it is missing without aborting the step. Render multiline override_reason content as part of the same markdown bullet, indenting continuation lines so they cannot become headings or separate list items. Preserve the existing promotion details and apply the same formatting to both notes_path and the job summary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SECURITY-ASSURANCE.md`:
- Around line 60-64: Update the authentication behavior description in
SECURITY-ASSURANCE.md to state that missing configured authentication does not
terminate the process; anonymous provider registration leaves Passport with zero
strategies, protected requests return 401, and /health reports 503. Replace the
ambiguous “fails closed” wording with this explicit service-boundary behavior.
- Around line 83-101: Update the security controls claims in the “Common
weakness and dependency controls” section to accurately describe
outbound-request protections: retain Axios timeout claims for app/release-notes,
response-size limiting for app/api/icons/fetch.ts, and remove unsupported
redirect and DNS-target enforcement claims for those paths. Preserve only CI and
100% coverage claims that are supported by the repository.
---
Nitpick comments:
In @.github/tests/release-cut-retry-workflow.test.ts:
- Around line 227-240: Extend the injection assertion in the release-step test
to inspect both `step.run` and the `with.command` shell body for direct
`inputs.soak_override_reason` interpolation. Confirm the `WorkflowStep` type
exposes the `with` field and widen that type if necessary, while preserving the
existing filtering and assertion behavior.
In @.github/tests/release-cut-soak-override.test.ts:
- Around line 229-238: Add a test alongside the existing soak_override_reason
validation cases that uses a reason whose trimmed length is exactly 20
characters, including surrounding whitespace, and assert the release step
succeeds. This should pin the inclusive boundary and verify trimming occurs
before the length check in runSourceStep.
In @.github/workflows/release-cut.yml:
- Around line 1136-1150: Harden the soak-override reason handling in the
release-body and job-summary blocks: validate that SOAK_OVERRIDE_REASON_PATH is
set and points to a readable file before reading it, and fall back to a safe
indication when it is missing without aborting the step. Render multiline
override_reason content as part of the same markdown bullet, indenting
continuation lines so they cannot become headings or separate list items.
Preserve the existing promotion details and apply the same formatting to both
notes_path and the job summary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f705a5e-940c-4a90-b85e-6d6dec2343ea
📒 Files selected for processing (8)
.github/tests/release-cut-retry-workflow.test.ts.github/tests/release-cut-soak-override.test.ts.github/workflows/release-cut.ymlGOVERNANCE.mdREADME.mdSECURITY-ASSURANCE.mdapps/web/scripts/marketing-performance.test.mjsapps/web/src/app/api/star-history/route.ts
…n aggregate CodeRabbit caught two overstatements in the assurance case, and both check out against the code. This document is the public evidence behind the OpenSSF Silver assessment, so an overstated control claim is the specific thing that criterion exists to prevent. "Registry auth, release-note, icon, webhook, and notification HTTP paths constrain protocols, redirects, DNS targets, and response sizes" reads as a blanket policy and is not one. Verified: - registries set a timeout and maxRedirects: 0, but no size cap and no metadata-address check - the HTTP trigger is the only path that resolves and refuses cloud metadata / link-local addresses - the agent Docker proxy has timeout, redirects, and size - icon fetch caps response size but follows redirects - release notes has timeouts only Replaced with a per-path table and an explicit sentence naming the gaps. "Authentication configuration fails closed" did not say what fails. It is the request boundary, not the process: with nothing configured and anonymous not confirmed, no Passport strategy registers, protected requests are rejected, and /health reports 503 (app/api/health.ts:34). - test(release-cut): pin both sides of the 20-character soak_override_reason boundary. The existing cases went from 1 character to 68, so flipping -lt to -le would have kept them all green.
|
Both findings check out against the code, fixed in c2183f2. Outbound-request controls. You're right that the blanket sentence doesn't hold. Verified each path:
Replaced the sentence with that table plus an explicit paragraph naming the gaps, rather than trimming the claim quietly. The HTTP trigger is the only path that resolves and refuses metadata/link-local addresses, which makes sense — it's the one taking an operator-supplied URL — but the document read as if all five did it. Auth fail-closed. Agreed it was ambiguous. It's the request boundary, not the process: no strategy registers, protected requests are rejected, and Nitpick on the 20-character boundary — taken. Good catch: the cases went 1 character straight to 68, so flipping Note for the record: this same wording is already on |
…laim The release-integrity section said GA promotes a previously tested candidate digest and stopped there. A reader would reasonably take that to mean the seven-day soak always held. It did not for v1.6.0, which was promoted at three days through the audited override. Leaving that out of the assurance case is the same class of problem as the outbound-request overstatement: the document is the public evidence behind the OpenSSF Silver assessment, so a gap between what it implies and what shipped is exactly what that criterion is meant to catch. Every detail stated is verified against release-cut.yml and the published release: the 604800s floor (line 294), the 20-character justification minimum (line 303), the warning naming the real age, and the reason reaching both the run summary and the release notes (line 1085). The v1.6.0 release body carries "Candidate age at promotion: 260253s (~3.0 days)" and the full reason.
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SECURITY-ASSURANCE.md (1)
92-111: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDocument all notification outbound paths and residual controls.
The table omits notification providers under
app/triggers/providers. Add each provider with its timeout, redirect handling, response-size cap, and metadata/link-local address control. Record third-party client behavior as unknown unless the dependency establishes the control.app/triggers/providers/ifttt/Ifttt.tsdoes not use the shared outbound timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SECURITY-ASSURANCE.md` around lines 92 - 111, Update the outbound-controls table in SECURITY-ASSURANCE.md to include every notification provider under app/triggers/providers, documenting timeout, redirect handling, response-size cap, and metadata/link-local address refusal for each. Mark third-party client controls as unknown unless the dependency establishes them, and accurately note that Ifttt.ts does not use the shared outbound timeout; update the surrounding narrative or evidence references as needed to reflect these entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@SECURITY-ASSURANCE.md`:
- Around line 92-111: Update the outbound-controls table in
SECURITY-ASSURANCE.md to include every notification provider under
app/triggers/providers, documenting timeout, redirect handling, response-size
cap, and metadata/link-local address refusal for each. Mark third-party client
controls as unknown unless the dependency establishes them, and accurately note
that Ifttt.ts does not use the shared outbound timeout; update the surrounding
narrative or evidence references as needed to reflect these entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1411031b-a092-4244-b2bd-aa8a9fb0d59e
📒 Files selected for processing (2)
.github/tests/release-cut-soak-override.test.tsSECURITY-ASSURANCE.md
… table CodeRabbit caught that the per-path table claimed to enumerate outbound controls individually while omitting app/triggers/providers entirely, which is the largest outbound surface in the app and the one the requirements section names by feature. Three rows added, split by how the control is established: nine providers pass the shared getOutboundHttpTimeoutMs(), five wrap a vendor SDK and inherit whatever it does, and IFTTT calls axios bare. That last one is a real defect, filed as #704 and cited from the table rather than papered over.
dev/v1.7 still advertised 1.6.0-rc.12 in the version badge. package.json says 1.6.0 and main's README already shows it; this branch is where the rest of the GA state is being forward-ported, so it belongs here too.
This reverts commit f98d8a7. The version badge isn't a standalone string: scripts/release-docs-identity.test.mjs pins it to RC_VERSION alongside the README highlights heading, site-config, the docs updates page, three API doc samples, the quickstart tag matrix, and the CHANGELOG compare links. Bumping the badge on its own made dev/v1.7 internally inconsistent, which is exactly what that test exists to catch, and it caught it. dev/v1.7 is still coherent at 1.6.0-rc.12. main carries the GA identity bump. Forward-porting that whole set is real work and gets its own change, not a one-line edit smuggled into this PR.
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Three things landed on
dev/v1.6during the GA push and never reacheddev/v1.7. Found by diffing the two branches after the GA sync madedev/v1.6andmaintree-identical.1. OpenSSF assurance evidence (
a8b33673, #699)GOVERNANCE.md,SECURITY-ASSURANCE.md, and the Best Practices badge in the README. This is the one that actually bites: drydock earned the Silver badge on 2026-08-12 specifically because the GA sync put that badge onmain's front page. The first v1.7 sync tomainwould have deleted it again and droppeddocumentation_achievementsback to Unmet, losing Silver.2. Star history through GraphQL (
38819656)v1.7 still had the REST
/stargazerspath. That endpoint 401s anonymously and, for a fine-grained token, demandscontents=write— far more than a public star chart should hold — so the route fell back on every request and getdrydock.com just said "loading". The GraphQL stargazers connection needs onlymetadata=read.3. Auditable soak override (
2b55ff4b)The
soak_override_reasondispatch input, its validation, and the 311-line test. The 604800s floor is untouched. Used once, for the v1.6.0 GA cut at 3.0 days; v1.7 should have the same escape hatch and the same audit trail.Deliberately not carried over
daf12292— v1.6.0 GA release identity and CHANGELOG. v1.7 produces its own.b98808e7— rc.13 release identity.bd6f598d— base-image digest bumps. The Dockerfile is already byte-identical;cee3a686covered it on this branch.#689and#683show up in the commit gap but were already forward-ported as#690and#686.Verification
apps/web/src/app/api/star-history/route.tsand.github/workflows/release-cut.ymlare byte-identical todev/v1.6.npm run test:workflows— 80/80.node --test apps/web/scripts/marketing-performance.test.mjs— 6/6.Changelog
soak_override_reasonsupport with validation, audit output, and release-note disclosure./stargazersrequests with GitHub GraphQL requests.Concerns
GITHUB_TOKENexists in every runtime that serves the star-history route.soak_override_reasonbefore release actions run.