Skip to content

Switch from === to hash_compare for time sensitive comparisons - #4743

Merged
ildyria merged 1 commit into
masterfrom
constant-time-compare
Sep 14, 2026
Merged

ildyria merged 1 commit into
masterfrom
constant-time-compare

Conversation

@ildyria

@ildyria ildyria commented Sep 14, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Security Improvements
    • Improved API key verification for face clustering, face detection, and NSFW detection requests with timing-safe comparisons.
    • Existing rejection of missing or empty API keys remains unchanged.

@ildyria
ildyria requested a review from a team as a code owner September 14, 2026 14:53
@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: e57a18b6-8ed2-4280-8f8f-b53dada953be

📥 Commits

Reviewing files that changed from the base of the PR and between 0b2ea9a and d1e0e18.

📒 Files selected for processing (3)
  • app/Http/Requests/Face/ClusterResultsRequest.php
  • app/Http/Requests/Face/FaceDetectionResultsRequest.php
  • app/Http/Requests/Nsfw/NsfwDetectionResultsRequest.php

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Three request authorization methods now compare configured and supplied API keys with hash_equals(). Existing empty-key rejection and other request logic remain unchanged.

Changes

API-key authorization

Layer / File(s) Summary
Timing-safe authorization comparisons
app/Http/Requests/Face/ClusterResultsRequest.php, app/Http/Requests/Face/FaceDetectionResultsRequest.php, app/Http/Requests/Nsfw/NsfwDetectionResultsRequest.php
The three authorize() methods use hash_equals() instead of strict string equality to compare API keys. Existing key guards remain unchanged.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to d1e0e

The timing-safe API-key checks preserve rejection of missing or empty credentials without introducing a runtime failure.

🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
  • Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.84%. Comparing base (0b2ea9a) to head (d1e0e18).

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ildyria
ildyria merged commit 394fdcb into master Sep 14, 2026
53 checks passed
@ildyria
ildyria deleted the constant-time-compare branch September 14, 2026 16:23
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.

1 participant