Skip to content

Remove restrictive cert validation#387

Merged
yanziz-nvidia merged 1 commit intomainfrom
blongs-nv/remove-unaccepted-cert-validation
Apr 11, 2026
Merged

Remove restrictive cert validation#387
yanziz-nvidia merged 1 commit intomainfrom
blongs-nv/remove-unaccepted-cert-validation

Conversation

@blongs-nv
Copy link
Copy Markdown
Contributor

@blongs-nv blongs-nv commented Apr 11, 2026

Update for the WebXR example' self-signed certificate check. Due to inconsistencies in how browsers could treat a fetch vs real wss connection, there could be false negatives on the cert acceptance check. This changes removes potentially blocking validation of self-signed certificates, keeping only the green successfully signed feedback but not blocking the Connect button if the probing fetch fails.

Summary by CodeRabbit, corrected by author

  • Style

    • Certificate acceptance link styling updated from red to blue color palette. (author: not the point of the change, blue was already the unverified color; failure to validate (old red status) just goes to unverified state rather than the negative state)
  • Refactor

    • Simplified certificate verification flow; the Connect button no longer requires waiting for verification completion before proceeding. (author: because there's no enforcement of cert validation, there's nothing to wait for)

@blongs-nv blongs-nv self-assigned this Apr 11, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 11, 2026

📝 Walkthrough

Walkthrough

This pull request removes certificate verification status tracking from the CloudXR WebXR client. The setupCertificateAcceptanceLink helper function now returns only a cleanup function instead of a controller object with verification methods. Exported interfaces CertStatusInfo and CertLinkController are removed, along with the onStatusChange callback parameter and internal verification state logic. In the CloudXR2DUI component, certificate status tracking is replaced with a single cleanup function reference, and the Connect button no longer blocks on certificate verification completion. CSS styling for the certificate acceptance component transitions from a red to blue color palette, and separate unverified state styling is eliminated.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Remove restrictive cert validation' directly matches the main change: eliminating blocking certificate validation that prevented connection when cert checks failed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch blongs-nv/remove-unaccepted-cert-validation

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

@blongs-nv blongs-nv requested a review from yanziz-nvidia April 11, 2026 00:17
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Author's note: this comment is incorrect about the purpose of the PR; this behavior is intended.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
deps/cloudxr/webxr_client/helpers/utils.ts (1)

404-429: ⚠️ Potential issue | 🟠 Major

Preserve the accepted badge across revalidation.

startVerification() always calls updateCertLink(), and updateCertLink() unconditionally resets the current URL to unverified before probing. checkCert() also clears the accepted state on any probe failure. After a cert has been accepted once, a later focus event or a false-negative fetch probe flips the UI back to “Click … to accept cert”, which defeats the PR’s goal of keeping the positive green feedback for the same URL.

Proposed fix
   async function checkCert(url: string): Promise<void> {
     if (url !== activeCertUrl) {
       return;
     }
@@
     try {
       await fetchFn(url, { signal: abortController.signal, mode: 'no-cors' });
       markAccepted(url);
     } catch (err) {
       if (err instanceof DOMException && err.name === 'AbortError') {
         return;
       }
-      markUnverified(url);
+      if (!accepted) {
+        markUnverified(url);
+      }
       console.warn(
         '[CloudXR] Certificate not yet accepted — cert polling errors for %s are expected.',
         url
       );
     }
   }
@@
   const updateCertLink = () => {
@@
     if (certRequired) {
       const effectiveIp = serverIpPopulated ? serverIp : new URL(location.href).hostname;
       const url = `https://${effectiveIp}:${port}/`;
+      const urlChanged = url !== activeCertUrl;
       activeCertUrl = url;
       certAcceptanceLink.style.display = 'block';
       certLink.href = url;
-      markUnverified(url);
+      if (urlChanged || !accepted) {
+        markUnverified(url);
+      }
     } else {
       activeCertUrl = null;
       accepted = false;

Also applies to: 435-455, 498-500

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@deps/cloudxr/webxr_client/helpers/utils.ts` around lines 404 - 429, The
accepted badge is being cleared during revalidation: modify
updateCertLink/startVerification and checkCert so that once markAccepted(url)
has been called for a given URL it is not overwritten by subsequent revalidation
or transient probe failures. Specifically, add a guard using the existing
activeCertUrl/accepted state so updateCertLink does not unconditionally set the
link to unverified if the URL equals the already-accepted URL, and change
checkCert to avoid calling markUnverified(url) for that same URL on non-abort
errors (only clear when the activeCertUrl changes or a confirmed rejection
occurs). Use the existing symbols (startVerification, updateCertLink, checkCert,
markAccepted, markUnverified, activeCertUrl, abortController) to locate and
apply these conditional checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@deps/cloudxr/webxr_client/helpers/utils.ts`:
- Around line 404-429: The accepted badge is being cleared during revalidation:
modify updateCertLink/startVerification and checkCert so that once
markAccepted(url) has been called for a given URL it is not overwritten by
subsequent revalidation or transient probe failures. Specifically, add a guard
using the existing activeCertUrl/accepted state so updateCertLink does not
unconditionally set the link to unverified if the URL equals the
already-accepted URL, and change checkCert to avoid calling markUnverified(url)
for that same URL on non-abort errors (only clear when the activeCertUrl changes
or a confirmed rejection occurs). Use the existing symbols (startVerification,
updateCertLink, checkCert, markAccepted, markUnverified, activeCertUrl,
abortController) to locate and apply these conditional checks.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a2be389d-899e-4447-95d4-8f522098079b

📥 Commits

Reviewing files that changed from the base of the PR and between 2658881 and ffd2144.

📒 Files selected for processing (3)
  • deps/cloudxr/webxr_client/helpers/utils.ts
  • deps/cloudxr/webxr_client/src/CloudXR2DUI.tsx
  • deps/cloudxr/webxr_client/src/index.html

@yanziz-nvidia yanziz-nvidia merged commit af72817 into main Apr 11, 2026
47 checks passed
@yanziz-nvidia yanziz-nvidia deleted the blongs-nv/remove-unaccepted-cert-validation branch April 11, 2026 02:26
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.

2 participants