fix(security): complete allowlist input validation audit - #233
Conversation
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
|
@parthrohit22 or @SHAURYAKSHARMA24, can u review this PR? |
parthrohit22
left a comment
There was a problem hiding this comment.
overall nice work on this @TFT444 the validator module is clean and I like that you're returning a
generic error instead of leaking the exception text. Pulled it down and ran it: your tests
pass, full suite and ruff are fine on my end.
One thing blocking though. ?category= is now case-sensitive, but the query underneath
still does LOWER(category) = LOWER(%s), so anything lowercase that worked before now
400s — ?category=network, ?category=storage, ?category=keyvault all break. You
handled this on severity with case="upper", category just missed it. Can't use .title()
since it'd mangle "Key Vault", so probably a lowercase-keyed lookup back to the canonical
value. Worth a test pinning it too.
Couple of smaller things while you're in there:
/api/compliance/<framework>still echoes the raw input back in its error and doesn't
go through the new validators — feels like it should, given it's in scope.validate_config()in the Sentinel CLI isn't wrapped, so a missing env var dumps a
traceback now. Also one bad record kills the whole batch, which is fine as a decision,
just worth documenting.
Rest looks good. Fix the category thing and I'm happy with it.
Reference : here's what I checked on my end: pulled the branch and ran your two new
test files (28 passed) plus the full suite — 506 passed, 3 skipped. The 5 failures I hit
are already there on dev from missing optional deps locally, nothing to do with your
changes. ruff check and ruff format --check both clean. Also poked at the routes
manually with some junk input — SQL-ish strings, script tags, non-canonical UUIDs — and
they all 400 before touching the DB, with nothing reflected back. Checked the category
allowlist against every CATEGORY constant in scanner/rules/ too, all 9 are covered.
That's how the case-sensitivity thing turned up — compared behaviour side by side
against dev.
Summary
Validation
ruff check .ruff format --check .511 passed, 3 skippedwith84.42%API/scanner coverageCloses #201