ldraney/mdview

Fork 0

Support relative markdown link navigation #4

Merged

ldraney merged 2 commits from feat/relative-link-navigation into main

2026-05-19 02:34:07 +00:00

ldraney commented

2026-05-19 02:30:29 +00:00

Owner

Summary

Clicking a relative .md link in rendered output now navigates to that file through mdview instead of 404ing. Enables doc trees with a README TOC linking into docs/.

Changes

Derive base_dir from MDVIEW_FILE parent directory -- all relative paths resolve from there
Add catch-all route GET /*.md that renders any markdown file under the base dir
Post-process rendered HTML to rewrite .md hrefs to mdview routes (external URLs untouched)
Path traversal guard: files must resolve within base_dir or get 403
"Back to root" link on sub-pages
CSS for the back link in gruvbox muted style

Test Plan

README with [link](docs/foo.md) -- click navigates to rendered foo.md
Back link returns to root
Mermaid diagrams work on sub-pages
../../../etc/passwd returns 403
External https:// links are not rewritten

Review Checklist

No hardcoded hex outside :root
Path traversal blocked
Follows ror-css-guide conventions

Closes #3

## Summary Clicking a relative `.md` link in rendered output now navigates to that file through mdview instead of 404ing. Enables doc trees with a README TOC linking into `docs/`. ## Changes - Derive `base_dir` from `MDVIEW_FILE` parent directory -- all relative paths resolve from there - Add catch-all route `GET /*.md` that renders any markdown file under the base dir - Post-process rendered HTML to rewrite `.md` hrefs to mdview routes (external URLs untouched) - Path traversal guard: files must resolve within `base_dir` or get 403 - "Back to root" link on sub-pages - CSS for the back link in gruvbox muted style ## Test Plan - [ ] README with `[link](docs/foo.md)` -- click navigates to rendered foo.md - [ ] Back link returns to root - [ ] Mermaid diagrams work on sub-pages - [ ] `../../../etc/passwd` returns 403 - [ ] External `https://` links are not rewritten ## Review Checklist - [x] No hardcoded hex outside `:root` - [x] Path traversal blocked - [x] Follows ror-css-guide conventions ## Related Notes Closes #3

ldraney added 1 commit

2026-05-19 02:30:30 +00:00

feat: support relative markdown link navigation

CI / scan_ruby (pull_request) Waiting to run

Details

CI / scan_js (pull_request) Waiting to run

Details

CI / lint (pull_request) Waiting to run

Details

2028459b59

Clicking a relative .md link (e.g. docs/user-stories.md) now
navigates to that file rendered through mdview instead of 404.

- Derive base_dir from MDVIEW_FILE parent directory
- Catch-all route for /*.md paths
- Post-process HTML to rewrite .md hrefs to mdview routes
- Path traversal guard (files must stay within base_dir)
- Back-to-root link on sub-pages

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

ldraney commented

2026-05-19 02:31:38 +00:00

Author

Owner

PR #4 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8, Redcarpet markdown rendering, CSS. Applying Ruby/Rails domain checks.

Path Traversal Guard (app/controllers/markdown_controller.rb:24-27)

The guard uses the standard File.expand_path + start_with? pattern:

@file_path = File.expand_path(relative, base_dir)
unless @file_path.start_with?(base_dir)

This has a prefix collision vulnerability. If base_dir resolves to /home/user/docs, then a crafted path resolving to /home/user/docs-evil/secret.md passes the start_with? check. The fix is to compare against base_dir + "/" (or use Pathname#ascend). In practice, since base_dir is derived from File.dirname(ENV.fetch("MDVIEW_FILE")) and this is a local CLI tool, the exploit requires a sibling directory with a matching prefix -- low probability but the fix is trivial and the pattern should be correct on principle.

ENV.fetch crash path (markdown_controller.rb:14)

base_dir uses ENV.fetch("MDVIEW_FILE") which raises KeyError if the env var is unset. The original code used ENV["MDVIEW_FILE"] with a nil check and a graceful 404. The new code will crash with an unhandled 500. This is a regression in error handling for the startup case.

File.join no-op (markdown_controller.rb:18)

File.join(params[:path]) with a single string argument just returns that string unchanged. Not a bug, but misleading -- it reads as if it is joining multiple path components. Either use params[:path] directly or add a comment explaining intent.

html_safe on file content (markdown_controller.rb:6)

@html = rewrite_md_links(html, @relative_dir).html_safe marks rendered markdown as safe HTML. Since this is a local CLI tool rendering local files you control, the XSS risk is self-inflicted only. Noted but not blocking.

Link rewriting regex (markdown_controller.rb:41)

The regex (<a\s[^>]*href=")([^"]*\.md)(") will miss:

Links with anchors: docs/foo.md#section
Links with query strings: docs/foo.md?v=1
Single-quoted href attributes

For a personal tool this is acceptable, but the anchor case (foo.md#heading) is a realistic markdown pattern that will silently fail to rewrite.

BLOCKERS

1. Path traversal prefix collision (Security)

start_with?(base_dir) without a trailing separator allows escape to sibling directories with matching prefixes. Fix: @file_path.start_with?(base_dir + "/") || @file_path == base_dir. This is the one security-relevant finding. While exploitability is low in a local CLI tool, the guard is the single security mechanism and should be correct.

NITS

ENV.fetch regression: Consider rescuing KeyError in base_dir or falling back to a graceful error message. The original nil-guard behavior was better UX for misconfiguration.
File.join(params[:path]): Replace with params[:path] directly, or use Pathname for clarity.
Regex anchor support: Extend the href regex to capture optional #fragment portions: ([^"]*\.md)(#[^"]*)?(") so that docs/setup.md#install links get rewritten correctly.
View logic: @relative_dir == "." ? @dirname : @relative_dir in the ERB template -- this conditional display logic could move to a helper method for clarity.
No automated tests: The test plan is manual-only. For a small CLI tool this is not blocking, but the path traversal guard specifically is a good candidate for a controller test (one happy path, one ../../../etc/passwd case, one prefix collision case).

SOP COMPLIANCE

Branch named after issue -- Branch is feat/relative-link-navigation, should be 3-relative-link-navigation per {issue-number}-{kebab-case-purpose} convention
PR body follows template (Summary, Changes, Test Plan, Related present)
Related references plan slug -- Closes #3 references the issue but no plan slug
No secrets committed
No scope creep -- all changes are directly related to relative link navigation
Commit messages are descriptive (PR title matches intent)

PROCESS OBSERVATIONS

Change failure risk: LOW. This is a small feature addition to a personal CLI tool with no production deployment pipeline. The path traversal fix is trivial.
Documentation: PR body is thorough with a manual test plan. The test plan checkboxes cover the key scenarios.
Branch naming: Minor SOP deviation. Not blocking for a personal tool repo, but worth maintaining the habit for consistency across all repos.

VERDICT: NOT APPROVED

One blocker: the path traversal guard has a prefix collision vulnerability that should be fixed before merge. The fix is a one-line change (base_dir + "/"). Once addressed, this is ready to merge.

## PR #4 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 8, Redcarpet markdown rendering, CSS. Applying Ruby/Rails domain checks. **Path Traversal Guard (app/controllers/markdown_controller.rb:24-27)** The guard uses the standard `File.expand_path` + `start_with?` pattern: ```ruby @file_path = File.expand_path(relative, base_dir) unless @file_path.start_with?(base_dir) ``` This has a **prefix collision vulnerability**. If `base_dir` resolves to `/home/user/docs`, then a crafted path resolving to `/home/user/docs-evil/secret.md` passes the `start_with?` check. The fix is to compare against `base_dir + "/"` (or use `Pathname#ascend`). In practice, since `base_dir` is derived from `File.dirname(ENV.fetch("MDVIEW_FILE"))` and this is a local CLI tool, the exploit requires a sibling directory with a matching prefix -- low probability but the fix is trivial and the pattern should be correct on principle. **ENV.fetch crash path (markdown_controller.rb:14)** `base_dir` uses `ENV.fetch("MDVIEW_FILE")` which raises `KeyError` if the env var is unset. The original code used `ENV["MDVIEW_FILE"]` with a nil check and a graceful 404. The new code will crash with an unhandled 500. This is a regression in error handling for the startup case. **File.join no-op (markdown_controller.rb:18)** `File.join(params[:path])` with a single string argument just returns that string unchanged. Not a bug, but misleading -- it reads as if it is joining multiple path components. Either use `params[:path]` directly or add a comment explaining intent. **html_safe on file content (markdown_controller.rb:6)** `@html = rewrite_md_links(html, @relative_dir).html_safe` marks rendered markdown as safe HTML. Since this is a local CLI tool rendering local files you control, the XSS risk is self-inflicted only. Noted but not blocking. **Link rewriting regex (markdown_controller.rb:41)** The regex `(<a\s[^>]*href=")([^"]*\.md)(")` will miss: - Links with anchors: `docs/foo.md#section` - Links with query strings: `docs/foo.md?v=1` - Single-quoted href attributes For a personal tool this is acceptable, but the anchor case (`foo.md#heading`) is a realistic markdown pattern that will silently fail to rewrite. ### BLOCKERS **1. Path traversal prefix collision (Security)** `start_with?(base_dir)` without a trailing separator allows escape to sibling directories with matching prefixes. Fix: `@file_path.start_with?(base_dir + "/") || @file_path == base_dir`. This is the one security-relevant finding. While exploitability is low in a local CLI tool, the guard is the single security mechanism and should be correct. ### NITS 1. **ENV.fetch regression**: Consider rescuing `KeyError` in `base_dir` or falling back to a graceful error message. The original nil-guard behavior was better UX for misconfiguration. 2. **File.join(params[:path])**: Replace with `params[:path]` directly, or use `Pathname` for clarity. 3. **Regex anchor support**: Extend the href regex to capture optional `#fragment` portions: `([^"]*\.md)(#[^"]*)?(")` so that `docs/setup.md#install` links get rewritten correctly. 4. **View logic**: `@relative_dir == "." ? @dirname : @relative_dir` in the ERB template -- this conditional display logic could move to a helper method for clarity. 5. **No automated tests**: The test plan is manual-only. For a small CLI tool this is not blocking, but the path traversal guard specifically is a good candidate for a controller test (one happy path, one `../../../etc/passwd` case, one prefix collision case). ### SOP COMPLIANCE - [ ] **Branch named after issue** -- Branch is `feat/relative-link-navigation`, should be `3-relative-link-navigation` per `{issue-number}-{kebab-case-purpose}` convention - [x] PR body follows template (Summary, Changes, Test Plan, Related present) - [ ] Related references plan slug -- `Closes #3` references the issue but no plan slug - [x] No secrets committed - [x] No scope creep -- all changes are directly related to relative link navigation - [x] Commit messages are descriptive (PR title matches intent) ### PROCESS OBSERVATIONS - **Change failure risk: LOW**. This is a small feature addition to a personal CLI tool with no production deployment pipeline. The path traversal fix is trivial. - **Documentation**: PR body is thorough with a manual test plan. The test plan checkboxes cover the key scenarios. - **Branch naming**: Minor SOP deviation. Not blocking for a personal tool repo, but worth maintaining the habit for consistency across all repos. ### VERDICT: NOT APPROVED One blocker: the path traversal guard has a prefix collision vulnerability that should be fixed before merge. The fix is a one-line change (`base_dir + "/"`). Once addressed, this is ready to merge.

ldraney added 1 commit

2026-05-19 02:32:12 +00:00

fix: path traversal prefix guard and ENV.fetch regression

CI / scan_ruby (pull_request) Waiting to run

Details

CI / scan_js (pull_request) Waiting to run

Details

CI / lint (pull_request) Waiting to run

Details

04bc64cff5

- Compare against base_dir + "/" to prevent sibling-dir escape
- Graceful 404 when MDVIEW_FILE is unset (was crashing with KeyError)
- Handle .md#anchor links in rewriting regex

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

ldraney commented

2026-05-19 02:33:12 +00:00

Author

Owner

PR #4 Re-Review

Re-review after fixes addressing the three findings from the initial review.

FIX VERIFICATION

1. Path traversal guard (was BLOCKER) -- FIXED

The guard now reads:

unless @file_path.start_with?("#{base_dir}/") || @file_path == entry

The trailing "/" on base_dir eliminates the prefix-confusion bypass (e.g., /home/user/docs-evil/ no longer passes a check against /home/user/docs). The || @file_path == entry correctly allows the root entry file itself, which is safe because entry is validated for existence immediately above. This is a correct and complete fix.

2. ENV.fetch crash regression -- FIXED

base_dir now uses ENV["MDVIEW_FILE"].to_s (safe on nil), and resolve_file does a graceful nil/existence check before proceeding:

entry = ENV["MDVIEW_FILE"]
unless entry && File.exist?(entry)
  render plain: "MDVIEW_FILE not set or missing", status: :not_found
  return
end

No crash path remains. Correct fix.

3. Regex missed .md#anchor links -- FIXED

Regex updated to (?:#[^"]*)? capturing optional fragment. The split/rejoin logic correctly preserves anchors during link rewriting. Verified correct.

DOMAIN REVIEW (Ruby on Rails)

Path security: The File.expand_path + start_with?("#{base_dir}/") pattern is the standard Rails approach to path traversal prevention. The guard runs in before_action, so no route bypasses it. Good.

Controller structure: Clean extraction of resolve_file as a before_action, render_markdown and rewrite_md_links as private helpers. Single responsibility per method. The before_action returning early on error (via render + return) correctly halts the action chain.

html_safe usage: @html = rewrite_md_links(html, @relative_dir).html_safe -- this is the one area worth noting. The HTML comes from Redcarpet rendering user-supplied markdown, then regex post-processing. Redcarpet does not sanitize by default. For a local single-user doc viewer tool, this is acceptable. If mdview ever serves untrusted markdown, a sanitization layer (e.g., Sanitize gem or Rails::HTML5::SafeListSanitizer) should be added before html_safe. Not a blocker for this use case.

Route constraint: get "/*path", to: "markdown#show", constraints: { path: /.*\.md/ } -- correctly limits the catch-all to .md paths only. Other asset routes and the root route are unaffected.

CSS: Uses CSS variables from :root exclusively (--color-muted, --color-link, --spacing-xs). No hardcoded hex values. Follows gruvbox token conventions from prior PR #2.

BLOCKERS

None. All three previous blockers are resolved.

NITS

base_dir when MDVIEW_FILE is nil: base_dir calls File.dirname(ENV["MDVIEW_FILE"].to_s) which yields File.dirname("") = ".". This is harmless because resolve_file returns early before base_dir is ever used in a path check when entry is nil. But a comment documenting this would make the safety reasoning explicit for future readers.
Route format constraint: The catch-all /*path with constraints: { path: /.*\.md/ } will also match paths like foo.markdown if someone renames, but will not match foo.MD (case-sensitive). For a personal doc viewer this is fine, but worth knowing.
Test coverage: The PR body has a manual test plan (path traversal returns 403, external links untouched, etc.) but no automated tests. For a personal tool this is not a blocker, but if the project grows, a request spec for the 403 path-traversal case would be high-value for regression prevention.

SOP COMPLIANCE

Branch named after issue -- branch is feat/relative-link-navigation not 3-relative-link-navigation. Minor deviation from {issue-number}-{kebab-case-purpose} convention.
PR body follows template -- Summary, Changes, Test Plan, Related all present
Related references issue -- Closes #3
No automated tests -- manual test plan only
No secrets committed
No scope creep -- all changes directly serve relative link navigation

PROCESS OBSERVATIONS

The review-fix-re-review loop worked cleanly here. All three findings from the initial review were addressed correctly in one pass. The path traversal fix is the most important -- the trailing-slash pattern is a well-known best practice and was applied correctly. Change failure risk is low; this is additive functionality with a proper guard rail.

VERDICT: APPROVED

## PR #4 Re-Review Re-review after fixes addressing the three findings from the initial review. ### FIX VERIFICATION **1. Path traversal guard (was BLOCKER) -- FIXED** The guard now reads: ```ruby unless @file_path.start_with?("#{base_dir}/") || @file_path == entry ``` The trailing `"/"` on `base_dir` eliminates the prefix-confusion bypass (e.g., `/home/user/docs-evil/` no longer passes a check against `/home/user/docs`). The `|| @file_path == entry` correctly allows the root entry file itself, which is safe because `entry` is validated for existence immediately above. This is a correct and complete fix. **2. ENV.fetch crash regression -- FIXED** `base_dir` now uses `ENV["MDVIEW_FILE"].to_s` (safe on nil), and `resolve_file` does a graceful nil/existence check before proceeding: ```ruby entry = ENV["MDVIEW_FILE"] unless entry && File.exist?(entry) render plain: "MDVIEW_FILE not set or missing", status: :not_found return end ``` No crash path remains. Correct fix. **3. Regex missed .md#anchor links -- FIXED** Regex updated to `(?:#[^"]*)?` capturing optional fragment. The split/rejoin logic correctly preserves anchors during link rewriting. Verified correct. ### DOMAIN REVIEW (Ruby on Rails) **Path security**: The `File.expand_path` + `start_with?("#{base_dir}/")` pattern is the standard Rails approach to path traversal prevention. The guard runs in `before_action`, so no route bypasses it. Good. **Controller structure**: Clean extraction of `resolve_file` as a `before_action`, `render_markdown` and `rewrite_md_links` as private helpers. Single responsibility per method. The `before_action` returning early on error (via `render` + `return`) correctly halts the action chain. **`html_safe` usage**: `@html = rewrite_md_links(html, @relative_dir).html_safe` -- this is the one area worth noting. The HTML comes from Redcarpet rendering user-supplied markdown, then regex post-processing. Redcarpet does not sanitize by default. For a local single-user doc viewer tool, this is acceptable. If mdview ever serves untrusted markdown, a sanitization layer (e.g., `Sanitize` gem or `Rails::HTML5::SafeListSanitizer`) should be added before `html_safe`. Not a blocker for this use case. **Route constraint**: `get "/*path", to: "markdown#show", constraints: { path: /.*\.md/ }` -- correctly limits the catch-all to `.md` paths only. Other asset routes and the root route are unaffected. **CSS**: Uses CSS variables from `:root` exclusively (`--color-muted`, `--color-link`, `--spacing-xs`). No hardcoded hex values. Follows gruvbox token conventions from prior PR #2. ### BLOCKERS None. All three previous blockers are resolved. ### NITS 1. **`base_dir` when `MDVIEW_FILE` is nil**: `base_dir` calls `File.dirname(ENV["MDVIEW_FILE"].to_s)` which yields `File.dirname("")` = `"."`. This is harmless because `resolve_file` returns early before `base_dir` is ever used in a path check when `entry` is nil. But a comment documenting this would make the safety reasoning explicit for future readers. 2. **Route format constraint**: The catch-all `/*path` with `constraints: { path: /.*\.md/ }` will also match paths like `foo.markdown` if someone renames, but will not match `foo.MD` (case-sensitive). For a personal doc viewer this is fine, but worth knowing. 3. **Test coverage**: The PR body has a manual test plan (path traversal returns 403, external links untouched, etc.) but no automated tests. For a personal tool this is not a blocker, but if the project grows, a request spec for the 403 path-traversal case would be high-value for regression prevention. ### SOP COMPLIANCE - [ ] Branch named after issue -- branch is `feat/relative-link-navigation` not `3-relative-link-navigation`. Minor deviation from `{issue-number}-{kebab-case-purpose}` convention. - [x] PR body follows template -- Summary, Changes, Test Plan, Related all present - [x] Related references issue -- `Closes #3` - [ ] No automated tests -- manual test plan only - [x] No secrets committed - [x] No scope creep -- all changes directly serve relative link navigation ### PROCESS OBSERVATIONS The review-fix-re-review loop worked cleanly here. All three findings from the initial review were addressed correctly in one pass. The path traversal fix is the most important -- the trailing-slash pattern is a well-known best practice and was applied correctly. Change failure risk is low; this is additive functionality with a proper guard rail. ### VERDICT: APPROVED