Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds spam protection to the snap report form by implementing a honeypot field and URL detection in comments. The honeypot is a hidden checkbox that legitimate users won't see or interact with, while bots are likely to fill it out. The backend silently rejects submissions that trigger either the honeypot or contain "http" in the comment field.
Changes:
- Added honeypot checkbox field (hidden via inline CSS) to the report snap modal form
- Implemented backend validation to silently reject spam submissions based on honeypot activation or URL presence in comments
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| webapp/store/snap_details_views.py | Added spam detection logic to check for honeypot field and URLs in comments before processing report submissions |
| templates/store/snap-details/_report_snap_modal.html | Added hidden honeypot checkbox field with label positioned off-screen using inline styles |
|
|
||
| fields = flask.request.form | ||
|
|
||
| # If the honeypot is activated or a URL is included in the comment, |
There was a problem hiding this comment.
The PR description is incomplete and does not meet the repository's required standards. According to the custom coding guidelines, every PR must include:
- Clear "How to QA" instructions with specific, actionable steps
- Link to the issue or card being addressed (currently shows "Fixes #" with no issue number)
- Completion of security considerations checklist (currently all unchecked)
- Brief explanation of what changed and why
These sections are currently empty or incomplete, making it difficult for reviewers to understand the context and properly test the changes.
| <label for="report-snap-confirm" style="position: absolute; top: -9999999px;">I agree</label> | ||
| <input id="report-snap-confirm" type="checkbox" name="confirm" style="position: absolute; top: -9999999px;" /> |
There was a problem hiding this comment.
The honeypot field lacks accessibility attributes that are important for screen reader users. While the field is intentionally hidden from visual users, it should include an aria-hidden="true" attribute to explicitly signal to assistive technologies that this field should be ignored. This ensures that screen reader users don't encounter confusion from a hidden "I agree" checkbox that appears unrelated to the form's stated purpose.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| # If the honeypot is activated or a URL is included in the comment, | ||
| # silently reject to avoid spam | ||
| if ( | ||
| "confirm" in fields and fields["confirm"] == "on" | ||
| ) or "http" in fields.get("comment", ""): | ||
| return flask.jsonify({"ok": True}), 200 |
There was a problem hiding this comment.
The spam detection implementation lacks test coverage. The repository has comprehensive Python test coverage for store endpoints (see tests/store/tests_details.py with 675 lines of tests), but this new security-critical functionality has no tests. Tests should verify:
- Legitimate reports without URLs are accepted
- Reports with the honeypot field checked are silently rejected
- Reports containing "http" in comments are rejected (though this behavior should be reconsidered as noted in another comment)
- The endpoint returns the correct response codes and JSON payloads in each scenario
Testing is particularly important for spam prevention logic to ensure it works as intended and doesn't create false positives.
| # If the honeypot is activated or a URL is included in the comment, | ||
| # silently reject to avoid spam | ||
| if ( | ||
| "confirm" in fields and fields["confirm"] == "on" | ||
| ) or "http" in fields.get("comment", ""): | ||
| return flask.jsonify({"ok": True}), 200 |
There was a problem hiding this comment.
The URL detection pattern is overly restrictive and will block legitimate reports. The check for "http" in the comment field will reject valid reports that include legitimate URLs (e.g., "This snap violates copyright of https://example.com"). Consider:
- Only blocking if the comment contains multiple URLs (spam pattern)
- Using a more sophisticated pattern to detect spam URLs vs. legitimate references
- Checking for "http://" or "https://" at word boundaries rather than just "http" as a substring
Additionally, "https" URLs will also be blocked since they contain "http" as a substring, making this check even more restrictive than likely intended.
…bility (#5590) * Initial plan * Add aria-hidden attribute to honeypot field for accessibility Co-authored-by: M7mdisk <43501040+M7mdisk@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: M7mdisk <43501040+M7mdisk@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| <label for="report-snap-email">Your email (optional)</label> | ||
| <input id="report-snap-email" type="email" name="email" placeholder="email@example.com" /> | ||
| <label for="report-snap-confirm" style="position: absolute; top: -9999999px;" aria-hidden="true">I agree</label> |
There was a problem hiding this comment.
Perhaps wrap these two fields in a div with the u-off-screen class, rather than using inline styles?
There was a problem hiding this comment.
didn't know that was a thing! thanks
| <label for="report-snap-email">Your email (optional)</label> | ||
| <input id="report-snap-email" type="email" name="email" placeholder="email@example.com" /> | ||
| <label for="report-snap-confirm" style="position: absolute; top: -9999999px;" aria-hidden="true">I agree</label> | ||
| <input id="report-snap-confirm" type="checkbox" name="confirm" style="position: absolute; top: -9999999px;" aria-hidden="true" /> |
There was a problem hiding this comment.
if we added a tabindex="-1" to the input, would it prevent bots from triggering the honey pot? currently tab navigation in the form is in part broken because it shifts focus to the hidden checkbox
There was a problem hiding this comment.
hmm, good point, I'll test it as is for now (mainly because thats how it was originally), I can then add this later down the line and see if we get more spam (I just need a point of reference for the time being)
Done
How to QA
Testing
Security
Issue / Card
Fixes #
Screenshots
UX Approval