Skip to content

Conversation

@tanmaykm
Copy link
Member

Adding checks to Endpoint urls to disallow:

  • path navigation. This would prevent API calls like GitForge.get_repo(forge, "JuliaLang", "../octocat/Hello-World") from succeeding. Helps avoid possible security loopholes.
  • newlines. This would prevent possible security loopholes using HTTP protocol.

Also added some tests.

Adding checks to `Endpoint` urls to disallow:
- path navigation. This would prevent API calls like `GitForge.get_repo(forge, "JuliaLang", "../octocat/Hello-World")` from succeeding. Helps avoid possible security loopholes.
- newlines. This would prevent possible security loopholes using HTTP protocol.

Also added some tests.
Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

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

I think this is fine for now. We can defer the decision about disallowing all whitespace characters to a later time.

Can you fix the typo on line 194 of src/forge.jl? After that, I think this is good to merge.

@tanmaykm
Copy link
Member Author

Thanks. Typo is fixed.

I have also added the change to disallow whitespace. Happy to roll it back if you think otherwise, but it does seem safer to go with that restrictiction now and we can always relax it if we find a genuine case for it later.

@nkottary nkottary merged commit 527332b into master Jun 16, 2025
8 checks passed
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.

4 participants