GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: fix defect path traversal tests by tniessen · Pull Request #50124 · nodejs/node

Conversation

@tniessen

@tniessen tniessen commented Oct 10, 2023

Copy link
Copy Markdown
Member

The test never actually tested what it claims to test because it did not properly insert separators before ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@tniessen tniessen added permission Issues and PRs related to the Permission Model. test Issues and PRs related to Node.js core tests and test infrastructure. security Issues and PRs related to security. labels Oct 10, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 10, 2023
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 10, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 10, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
tniessen requested a review from RafaelGSS October 10, 2023 16:44
anonrig
anonrig approved these changes Oct 10, 2023
View reviewed changes
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Oct 11, 2023
CI Reliability 2023-10-11 nodejs/reliability#690
Open
30 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen tniessen added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 11, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54696/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Uzlopak
Uzlopak approved these changes Oct 11, 2023
View reviewed changes
Trott
Trott previously approved these changes Oct 11, 2023
View reviewed changes
@Trott
Trott dismissed their stale review October 11, 2023 15:40

Actually, I'm not 100% sure I understand the motivation of the change and would rather wait for Rafael to weigh in

@tniessen

tniessen commented Oct 11, 2023

Copy link
Copy Markdown
Member Author

I'm not 100% sure I understand the motivation of the change

The existing test seems to assume that

const allowedFolder = tmpdir.resolve('subdirectory/');

keeps the trailing /, however, tmpdir.resolve() simply uses path.resolve() internally, which discards trailing separators, so allowedFolder ends with subdirectory, not subdirectory/.

The other JavaScript file then constructs paths as follows:

const traversalPath = allowedFolder + '../file.md';

Therefore, traversalPath ends with subdirectory../file.md, which is not a path traversal scenario. Adding a separator in front of .. solves this and should lead to an actual attempt at path traversal (i.e., subdirectory/../file.md).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
test: fix defect path traversal tests
953131a 
The test never actually tested what it claims to test because it did not
properly insert separators before `..`.
@tniessen
tniessen force-pushed the test-fix-pm-traversal branch from e159f6e to 953131a Compare October 15, 2023 13:42
@tniessen

tniessen commented Oct 15, 2023

Copy link
Copy Markdown
Member Author

Rebased due to a conflict with f447a46.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
tniessen requested review from RafaelGSS and removed request for RafaelGSS October 15, 2023 13:42
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 15, 2023
RafaelGSS
RafaelGSS approved these changes Oct 15, 2023
View reviewed changes
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 15, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Oct 16, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54851/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen tniessen added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 16, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 16, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 0c56962 into nodejs:main Oct 16, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 16, 2023

Copy link
Copy Markdown
Collaborator

Landed in 0c56962
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request Oct 23, 2023
@tniessen @targos
test: fix defect path traversal tests
ae905a8 
The test never actually tested what it claims to test because it did not
properly insert separators before `..`.

PR-URL: #50124
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
@targos targos mentioned this pull request Oct 23, 2023
v21.1.0 release proposal #50335
Merged
targos pushed a commit that referenced this pull request Nov 11, 2023
@tniessen @targos
test: fix defect path traversal tests
7cd406a 
The test never actually tested what it claims to test because it did not
properly insert separators before `..`.

PR-URL: #50124
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
@targos targos mentioned this pull request Nov 12, 2023
v20.10.0 release proposal #50682
Merged
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes
@Trott Trott Trott left review comments
+1 more reviewer
@Uzlopak Uzlopak Uzlopak approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model. security Issues and PRs related to security. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@tniessen @nodejs-github-bot @Trott @anonrig @Uzlopak @RafaelGSS
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.