Skip to content

Warn on relative paths in secure_path#1558

Open
WaterWhisperer wants to merge 1 commit into
trifectatechfoundation:mainfrom
WaterWhisperer:warn-relative-secure-path
Open

Warn on relative paths in secure_path#1558
WaterWhisperer wants to merge 1 commit into
trifectatechfoundation:mainfrom
WaterWhisperer:warn-relative-secure-path

Conversation

@WaterWhisperer
Copy link
Copy Markdown
Contributor

Fixes: #1153

@WaterWhisperer
Copy link
Copy Markdown
Contributor Author

WaterWhisperer commented Apr 21, 2026

I extended the existing secure_path tests instead of adding separate new ones for a small behavior change.

@WaterWhisperer WaterWhisperer force-pushed the warn-relative-secure-path branch from e13ed15 to 8e40223 Compare April 21, 2026 14:29
Comment thread src/common/context.rs Outdated
Comment thread src/common/context.rs Outdated
Comment thread src/common/context.rs Outdated
@squell
Copy link
Copy Markdown
Member

squell commented Apr 22, 2026

Suggestion: if user_warn! is used, that means visudo cannot act on this diagnostic.

Another approach could be to add this sanity check in sudoers/mod.rs and push it on the vector containing the various diagnostics (similar to how circular aliases are complained about).

@WaterWhisperer WaterWhisperer force-pushed the warn-relative-secure-path branch from 8e40223 to f89934e Compare April 23, 2026 03:40
@WaterWhisperer
Copy link
Copy Markdown
Contributor Author

Suggestion: if user_warn! is used, that means visudo cannot act on this diagnostic.

Another approach could be to add this sanity check in sudoers/mod.rs and push it on the vector containing the various diagnostics (similar to how circular aliases are complained about).

I reworked this into the sudoers diagnostics path so visudo sees it too.

One thing I'm a bit confused: under the visudo pipeline this ends up being shown as syntax error: ..., even though it is a diagnostic about secure_path(and like recursive alias). Is that the intended presentation?

@squell
Copy link
Copy Markdown
Member

squell commented Apr 23, 2026

One thing I'm a bit confused: under the visudo pipeline this ends up being shown as syntax error: ..., even though it is a diagnostic about secure_path(and like recursive alias). Is that the intended presentation?

That's a good point. We're open to changing that error prefix to something better (could be more generic like "error" or less lazy like "configuration error"). Technically, you could argue it is also not a syntax error if someone would write Defaults !flerbage.

@WaterWhisperer
Copy link
Copy Markdown
Contributor Author

That's a good point. We're open to changing that error prefix to something better (could be more generic like "error" or less lazy like "configuration error"). Technically, you could argue it is also not a syntax error if someone would write Defaults !flerbage.

Makes sense. The wording still feels a little imperfect to me, but I think it's acceptable now. A more general prefix improvement could be handled separately and may need more discussions.

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.

Add warning when a relative path is part the path used for resolving commands

3 participants