Make the Security Scanning as a blocking validator#266
Make the Security Scanning as a blocking validator#266Xpirix wants to merge 7 commits intoqgis:masterfrom
Conversation
…t and secrects issues
|
Thank you for this work @Xpirix , I like very much the proposed flow. I'll took a deeper look next week but the overall looks good (within the limit of my skills in django 🙃 ) |
|
My pleasure @troopa81. |
troopa81
left a comment
There was a problem hiding this comment.
I take a better look and it looks like good work 👍
I didn't see anything wrong although it would certainly require a thorough review from someone who has the appropriate expertise.
|
Thanks for the review and the feedback @troopa81 . |
|
Hello, Thanks for this work which contributes to increase significantly the QGIS ecosystem quality.
Well, I would say: it depends of which error / violation codes. Typically, in the plugins bootstrapped with the QGIS Plugin Templater, we chose to enforce some flake8 and flake8-qgis checks in git hooks (https://gitlab.com/Oslandia/qgis/template-qgis-plugin/-/blob/master/plugin_%7B%7Bcookiecutter.plugin_name_slug%7D%7D/.pre-commit-config.yaml?ref_type=heads#L65-76) i.e. you can't push commits which are breaking those rules considering those as critical. For plugins requiring a high code quality, we also enforce docstring, code complexity, etc. which are generally non blocking. Just my 2 cts. |
Guts
left a comment
There was a problem hiding this comment.
A quick review to make the project more consistent with what it does on the Python linters/PEP compliance side :).
Thanks for the suggestion @Guts . I have no strong opinions on this. I could also implement that as well. However, I wonder if it wouldn't match the initial suggestion made in the QEP https://github.com/qgis/QGIS-Enhancement-Proposals/blob/master/qep-408-plugins-security-validator.md#stage-2-validation-results-after-checks-complete. So it probably also needs to be updated accordingly? |
|
Hi. Yes I would love this check to be added but lets update the QEP or (probably better) create a new QEP to publicise this proposed change. |
I think it would be better to have another QEP and another vote. IMHO, this PR is doing what QEP 408 was proposing so I'd would be in favor of merging it as is (as long as somebody would approve the technical part of it). |
dimasciput
left a comment
There was a problem hiding this comment.
Hi @Xpirix, I only have a few feedback points on your PR. I don’t think they’re critical, but it might be good to check them.
|
Thanks so much for the feedback @dimasciput . I will fix them accordingly. |
|
Thanks so much for your inputs, everyone. I have addressed all reviews/suggestion so far. I will merge/deploy this early next week if there are no more comments. |
Closes #216 · Implements QEP-408
Cc @m-kuhn @troopa81 @3nids @Gustry
Summary
This PR introduces an asynchronous, blocking security and quality validation pipeline for all
plugin uploads. Rather than running checks synchronously and treating results as purely
informational, security scans now gate plugin availability: a version that fails critical checks
is blocked from download and approval until a clean version is uploaded.
What Changed
New Upload Flow
Before this PR, plugins were immediately available after upload and security scan results were
advisory only. The new flow is:
approval.
that checks are running.
manual approval.
All three upload methods are covered: the web upload form, the token-based REST endpoint, and
XML-RPC.
Blocking vs. Non-blocking Checks
Manual Re-scan
Plugin editors can trigger a re-scan at any time from the Security tab on the version detail
page. Re-scans refresh the results and timestamp but are informational only — they do not
change the validation status or approval state. To clear a blocked version, a new version must
be uploaded.