Skip to content

fix: secure logo switching and section navigation - #105

Merged
D3SOX merged 1 commit into
masterfrom
secure-theme-and-section-navigation
Oct 2, 2026
Merged

D3SOX merged 1 commit into
masterfrom
secure-theme-and-section-navigation

Conversation

@D3SOX

@D3SOX D3SOX commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

CodeQL alerts 1 and 2 flag DOM attribute and dropdown values being reused as URLs during logo switching and section navigation.

Keep logo asset URLs in <picture> markup and switch their media conditions when the theme changes. Have section pickers follow matching same-origin links already rendered in their navigation container. Fragment scrolling and focus remain intact.

Validation: Bun tests pass, the modified Astro files compile without diagnostics, and an offline browser fixture verifies Light/Dark/Auto logo loading and fragment focus. Added regression tests cover allowed navigation, rejected destinations, and theme switching.

Created with GPT-6.1-Sol through the Codex harness in T3 Code.

Review in cubic

Remove DOM-derived URL assignments reported by CodeQL while preserving theme switching and page-index navigation.

Security alerts: https://github.com/OpenTubeX/opentubex.github.io/security/code-scanning/1 and https://github.com/OpenTubeX/opentubex.github.io/security/code-scanning/2

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @D3SOX, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 6 days and 1 hour by commenting @sourcery-ai review. Upgrade to get a review now.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4d15dc2f-08f0-42a2-b296-111425281c73

📥 Commits

Reviewing files that changed from the base of the PR and between e35a760 and 95e863e.

📒 Files selected for processing (6)
  • public/signal/theme.js
  • src/layouts/SignalLayout.astro
  • src/pages/index.astro
  • src/scripts/jump-select.ts
  • tests/jump-select.test.ts
  • tests/theme-logos.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The logo markup now uses <picture> sources for light and dark assets, while theme synchronization toggles source media. The jump selector now follows matching same-origin navigation links for single-slash paths and retains guarded fragment navigation. Tests cover both behaviors.

Changes

Theme-Specific Logos

Layer / File(s) Summary
Picture sources and theme selection
src/layouts/SignalLayout.astro, src/pages/index.astro, public/signal/theme.js, tests/theme-logos.test.ts
The brand and hero logos use light and dark picture sources. Theme synchronization toggles source media. Tests check media values for light, dark, and auto preferences, and verify that source URLs remain unchanged.

Jump Selector Navigation

Layer / File(s) Summary
Path and fragment navigation
src/scripts/jump-select.ts, tests/jump-select.test.ts
The handler clicks a matching same-origin navigation link for a single-slash path and returns without navigation when no matching link exists. Fragment handling validates and decodes the target, then updates the hash and focuses the target when found. Tests cover these cases.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 95e86

No outstanding issue is identified that would prevent merging the logo and navigation changes after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 95e86

The inspected changes narrow browser URL handling without adding privileges or broader access. No introduced security issue was identified in these flows, but overall security and deployment coverage remains incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected changed sinks are logo loading and navigation in the current browser document. These handlers introduce no credential, tenant, or service authority; their directly affected scope is the rendered website UI.

Trust Boundaries and Controls

  • observed — A dropdown value alone no longer authorizes path navigation. It must begin with a single slash, match an anchor's raw href in the closest navigation container, and pass the browser-resolved same-origin check. Missing or mismatched anchors cause no navigation.
  • observed — Theme preferences are normalized to light, dark, or auto. Logo synchronization converts a theme comparison into only the fixed media values all or not all, removing the previous DOM-attribute-to-image-URL assignment.

Resilience and Maintainability Implications

  • inferred — Repeated logo synchronization is idempotent for a resolved theme. Before initialization or after interrupted synchronization, the inspected picture markup still references only its imported assets; inconsistent media selection can affect presentation but does not restore runtime URL authority. Preference-storage failure handling is unchanged by this PR.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description directly explains the security fixes, logo switching changes, section navigation behavior, validation, and regression tests.
Title check ✅ Passed The title clearly summarizes the two main changes: secure logo switching and section navigation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@D3SOX

D3SOX commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

Note

🤖 GPT-6.1-Sol responding on behalf of Nico

Leaving the docstring-coverage warning unchanged: this repository does not require docstrings, and these small functions follow the existing code conventions. Adding boilerplate solely to meet the bot's coverage percentage would not clarify this security fix. CodeQL and the site check pass, and the review reported no actionable findings.

@D3SOX
D3SOX merged commit 955dbe3 into master Oct 2, 2026
7 checks passed
@D3SOX
D3SOX deleted the secure-theme-and-section-navigation branch October 2, 2026 08:14
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