pwnshop: require explicit relative paths - #106
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 211a4cd8f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if start.is_file(): | ||
| start = start.parent | ||
| for candidate in (start, *start.parents): | ||
| if (candidate / ".git").exists(): |
There was a problem hiding this comment.
.git artifacts when locating repo root
_find_git_root returns the first ancestor containing a .git entry, so if a challenge directory intentionally contains a .git artifact (a realistic CTF case), render() will treat that challenge folder as the repo root and then fail template.relative_to(git_root / "challenges"), breaking rendering/build for that challenge. The root detection should verify it found the actual project root (e.g., by checking expected repo layout) instead of any .git path.
Useful? React with 👍 / 👎.
|
@zardus I'm curious what you think about something like this? Trivial codex generation, so no hard feelings if you hate it, but it feels like being explicit is better than the implicit recursive lookup. Alternatively, we could lay out the full paths starting from I was put on to this idea because the current existing recursive lookup fails it seems in the case where A extends B which extends C (not exactly sure of the specifics, but I ran into a bug that required using unique names for files because it was failing). We could of course try to go down the route of fixing this instead if you really like the implicit behavior, but I think I personally lean towards explicit. And of course we can merge #83 before we consider this, just figured it's about as easy to open a PR to do this as it is to write up the idea these days. |
|
I think this is a great idea
…On Thu, Feb 12, 2026 at 9:10 PM Connor Nelson ***@***.***> wrote:
*ConnorNelson* left a comment (pwncollege/challenges#106)
<#106 (comment)>
@zardus <https://github.com/zardus> I'm curious what you think about
something like this? Trivial codex generation, so no hard feelings if you
hate it, but it feels like being explicit is better than the implicit
recursive lookup.
Alternatively, we could lay out the full paths starting from challenges/,
e.g. {% extends "challenges/common/Dockerfile.j2" %} or {%
challenges/computing-101/common/Dockerfile.j2 %} if you don't like ../../;
not sure which I prefer.
I was put on to this idea because the current existing recursive lookup
fails it seems in the case where A extends B which extends C (not exactly
sure of the specifics, but I ran into a bug that required using unique
names for files because it was failing). We could of course try to go down
the route of fixing this instead if you really like the implicit behavior,
but I think I personally lean towards explicit.
And of course we can merge #83
<#83> before we consider
this, just figured it's about as easy to open a PR to do this as it is to
write up the idea these days.
—
Reply to this email directly, view it on GitHub
<#106 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2LHF3H4ANMKT3K7S2HUVL4LTM43AVCNFSM6AAAAACU5VTK52VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTQOJTGE2DIMZRGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
88b6649 to
d4a4975
Compare
2 participants
This simplifies
pwnshopto no longer do an implicit lookup through ancestor paths to find the firstcommon/for "common" directories. Instead, we require explicit paths. As an example, instead of{% include "common/server.py" %}, it is now{% include "../../common/server.py" %}(assumingcommon/is two directories up).