Skip to content

chore: minor hardening pass (XSS, concurrency, public-only comment)#2

Merged
shigechika merged 2 commits into
mainfrom
chore/minor-hardening
Apr 22, 2026
Merged

chore: minor hardening pass (XSS, concurrency, public-only comment)#2
shigechika merged 2 commits into
mainfrom
chore/minor-hardening

Conversation

@shigechika

Copy link
Copy Markdown
Owner

Summary

Three small, independent hardening tweaks bundled into one PR:

  • scripts/collect.sh — Strengthen the comment around the ?type=public filter. It is the only gate that stops this tool from fetching private-repo traffic, even when the PAT happens to grant broader access. Future contributors should know not to weaken it casually.
  • .github/workflows/collect.yml — Add a collect-traffic concurrency group. Cron and manual workflow_dispatch runs now queue rather than race on data/traffic.json. cancel-in-progress: false because each tick still carries fresh traffic data worth preserving.
  • docs/index.html — Replace innerHTML with DOM APIs in renderTotals and the error fallback. The data is owner-controlled (your own repo's traffic.json), so the practical risk is tiny, but textContent / createElement is the right idiom and removes the XSS surface entirely.

Test plan

  • CI: Collect Traffic Data workflow passes on this branch (the workflow change applies on the default branch only, but YAML syntax can be checked here)
  • Local: open docs/index.html against a fork URL — totals table still renders, dark/light mode unaffected
  • After merge: trigger gh workflow run collect.yml and confirm a second concurrent dispatch queues instead of running in parallel

🤖 Generated with Claude Code

shigechika and others added 2 commits April 23, 2026 07:03
- scripts/collect.sh: clarify that `?type=public` is the sole gate
  preventing private-repo traffic from being fetched or stored.
- .github/workflows/collect.yml: add a `collect-traffic` concurrency
  group so cron and manual dispatches queue instead of racing on
  data/traffic.json.
- docs/index.html: replace `innerHTML` with DOM APIs in renderTotals
  and the error fallback to remove a low-risk XSS surface.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The error fallback declared `const main = document.querySelector("main")`,
shadowing the outer async `main` function. Behaviour is unchanged but
future readers should not have to untangle the shadowing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shigechika shigechika merged commit 8d00197 into main Apr 22, 2026
1 check passed
@shigechika shigechika deleted the chore/minor-hardening branch April 22, 2026 22:15
shigechika added a commit that referenced this pull request Apr 26, 2026
* docs(security): add SECURITY.md and SRI hashes for CDN scripts

Following the formal security review pass requested in #8. Findings:

- Token handling, public-only invariant, workflow permissions, shell
  injection, repoSlug() coercion, and dashboard XSS surface all check
  out (most were already addressed by PRs #2, #11, #12).
- One finding remained: docs/index.html loaded chart.js and
  chartjs-adapter-date-fns from cdn.jsdelivr.net without integrity
  attributes, so a CDN/NPM compromise could swap the file at the same
  URL. Added SHA-384 SRI hashes plus crossorigin="anonymous" to both
  <script> tags.

SECURITY.md captures the trust model (who can read/write what),
private-vulnerability-reporting URL, the public-only invariant and
where it's enforced, workflow trust boundaries, the frontend supply
chain story, the Protect main ruleset, and a checklist for fork users
covering PAT scope / Pages visibility / private-repo safety / secret
naming / branch protection portability / rename detection.

Closes #8

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(security): correct SRI hashes for CDN scripts

The integrity values in the previous commit were computed from a
transient bad curl response — both differed from the bytes jsdelivr
actually serves. Verified by hashing each file three times in a row
(stable) and grep-checking docs/index.html.

Without this fix the browser would have rejected both <script> tags
on SRI mismatch, leaving the dashboard blank.

Correct hashes:
- chart.js@4.4.1                    sha384-9nhczxUqK87bcKHh20fSQcTGD4qq5GhayNYSYWqwBkINBhOfQLg/P5HG5lF1urn4
- chartjs-adapter-date-fns@3.0.0    sha384-cVMg8E3QFwTvGCDuK+ET4PD341jF3W8nO1auiXfuZNQkzbUUiBGLsIQUE+b1mxws

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant