fix(tm-core): slugify tag before using it in file paths (CWE-23 path traversal)#1712
Open
zied-jlassi wants to merge 2 commits into
Open
fix(tm-core): slugify tag before using it in file paths (CWE-23 path traversal)#1712zied-jlassi wants to merge 2 commits into
zied-jlassi wants to merge 2 commits into
Conversation
The complexity-report reader (ComplexityReportManager.getReportPath) and the per-task file generator (TaskFileGeneratorService) interpolated the `tag` value straight into a filename. Tags are user/agent/config controllable (CLI `--tag`, MCP `tag` argument, `.taskmaster/state.json` `currentTag`, `TASKMASTER_TAG`), so a tag such as `../../../../../escape/x` escaped the `.taskmaster/` directory — an out-of-project file read (complexity report) and write (task markdown). The legacy JS path slugified the tag (`slugifyTagForFilePath`), but the TypeScript rewrite dropped that step, and the `sanitizeTag` helper on the storage interface was never wired in. This adds a shared `slugifyTagForFilePath` helper (mirroring the legacy behavior) and applies it at both path builders, plus the orphan-cleanup pattern so writes and cleanup stay in sync. Legitimately named tags (which already match `^[a-zA-Z0-9_-]+$`) are unaffected. Adds unit tests with a negative oracle (`../`, `..\`, NUL, URL-encoded must be neutralized; `feature-v2` preserved) and a changeset. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>
🦋 Changeset detectedLatest commit: cdc59a1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new ChangesTag Path Traversal Security Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/tm-core/src/common/utils/tag-path.spec.ts (1)
63-71: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd test for all-invalid-char input.
The edge case where
tagNamecontains only invalid characters (e.g.,"../../../") should be tested to ensure it returns the fallback rather than an empty string (if the implementation fix is applied).Suggested test case
describe('empty / invalid input', () => { it('returns a fallback for empty string', () => { expect(slugifyTagForFilePath('')).toBe('unknown-tag'); }); it('returns a fallback for null/undefined', () => { expect(slugifyTagForFilePath(null)).toBe('unknown-tag'); expect(slugifyTagForFilePath(undefined)).toBe('unknown-tag'); }); + + it('returns a fallback when all characters are invalid', () => { + expect(slugifyTagForFilePath('../../../')).toBe('unknown-tag'); + expect(slugifyTagForFilePath('///')).toBe('unknown-tag'); + }); });🤖 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 `@packages/tm-core/src/common/utils/tag-path.spec.ts` around lines 63 - 71, Add a test case in the "empty / invalid input" describe block for the slugifyTagForFilePath function to verify that when tagName contains only invalid characters (such as "../../../"), the function returns the fallback value "unknown-tag" instead of an empty string. This test should be placed alongside the existing tests for empty string and null/undefined inputs to ensure all edge cases are covered.
🤖 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 `@packages/tm-core/src/common/utils/tag-path.ts`:
- Around line 42-51: After the series of string transformations (replace, remove
leading/trailing hyphens, collapse, lowercase, substring) that process the
tagName, add a final validation check to handle the edge case where all
transformations result in an empty string. If the final result is empty, return
the fallback value 'unknown-tag', otherwise return the processed result. This
prevents malformed filenames when the input consists entirely of invalid
characters.
---
Nitpick comments:
In `@packages/tm-core/src/common/utils/tag-path.spec.ts`:
- Around line 63-71: Add a test case in the "empty / invalid input" describe
block for the slugifyTagForFilePath function to verify that when tagName
contains only invalid characters (such as "../../../"), the function returns the
fallback value "unknown-tag" instead of an empty string. This test should be
placed alongside the existing tests for empty string and null/undefined inputs
to ensure all edge cases are covered.
🪄 Autofix (Beta)
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
Run ID: 76c252a4-e64f-4315-b244-9fbb9e718726
📒 Files selected for processing (6)
.changeset/fix-tag-path-traversal.mdpackages/tm-core/src/common/utils/index.tspackages/tm-core/src/common/utils/tag-path.spec.tspackages/tm-core/src/common/utils/tag-path.tspackages/tm-core/src/modules/reports/managers/complexity-report-manager.tspackages/tm-core/src/modules/tasks/services/task-file-generator.service.ts
Addresses CodeRabbit review on eyaltoledano#1712: a tag composed entirely of invalid characters (e.g. `../../../`) collapses to an empty slug, which would produce a malformed `task-complexity-report_.json` suffix. Return the same `unknown-tag` sentinel already used for empty/non-string input, and cover it with tests. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>
Author
|
Good catch on the empty-slug edge case — a tag that is entirely invalid characters (e.g. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
tagis interpolated raw into filenames in twotm-corepath builders:ComplexityReportManager.getReportPath→task-complexity-report_<tag>.json(read)TaskFileGeneratorService.getTaskFileName→task_<id>_<tag>.md(write)Tags are user/agent/config controllable (CLI
--tag, MCPtagargument,.taskmaster/state.jsoncurrentTag,TASKMASTER_TAG), and none of these entrypoints validates the value on the read/generate path (
validateTagNameonly runson create/rename/copy). So a tag like
../../../../../escape/xescapes.taskmaster/— an out-of-project file read (complexity report) and write(task
.md). CWE-22/23/73.This is a parity regression: the legacy JS path slugified the tag
(
slugifyTagForFilePathinscripts/modules/utils.js); the TypeScript rewritedropped it, and the
sanitizeTaghelper on the storage interface was never wired in.Fix: a shared
slugifyTagForFilePathhelper (mirroring the legacy behavior),applied at both path builders and at
cleanupOrphanedFilesso written names andthe orphan-cleanup pattern stay in sync. Sanitizing at the path-building chokepoint
covers all three entry vectors at once. Legitimately named tags (already
^[a-zA-Z0-9_-]+$) are unaffected beyond lowercasing. Honest severity: Medium —the extension is forced (
.md/.json), so this is not arbitrary-file overwrite or RCE.Related Issues
Fixes #1711.
How to Test This
npm run test -w @tm/core -- tag-path.spec.ts task-file-generator.service.spec.tsExpected result: all tests pass, including the new negative-oracle cases where
../../x,..\..\x, NUL and..%2f..are neutralized whilefeature-v2is preserved.Manual before/after (project root
/work/project): tag../../../../../escape/secretresolved outside the project before the fix and resolves to
…/.taskmaster/reports/task-complexity-report_escape-secret.json(contained) after.Contributor Checklist
npm run changesetnpm test(vitest 26/26 on the new + touched specs;tsc --noEmitclean)npm run format-check(biome, no changes)Changelog Entry
Fix path traversal via an unsanitized
tagin the complexity-report reader and task-file generator (tags are now slugified before use in file paths).Disclosure: this fix was AI-assisted and human-reviewed; it was tested on the repo's
own vitest/tsc/biome before submission. Happy to follow whatever disclosure format you prefer.
Summary by CodeRabbit
Release Notes
../, slashes, and other unsafe characters from escaping the intended directory.