FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

docs(CORE-1128): document no-cert-pinning risk-acceptance decision by jeanfbrito · Pull Request #3396 · RocketChat/Rocket.Chat.Electron · GitHub

/ Rocket.Chat.Electron Public

docs(CORE-1128): document no-cert-pinning risk-acceptance decision - #3396

Merged
jeanfbrito merged 1 commit into
masterfrom
security/CORE-1128-cert-pinning-docs
Jul 9, 2026
ConversationCommits (1)ChecksFiles changed
Merged

docs(CORE-1128): document no-cert-pinning risk-acceptance decision#3396
jeanfbrito merged 1 commit into
masterfrom
security/CORE-1128-cert-pinning-docs

Conversation

jeanfbrito commented Jul 8, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

Summary

  • CORE-1128 (2022 pentest finding) asked for TLS certificate pinning on the auto-updater's connections.
  • Investigated and decided against implementing pinning — documented the rationale in docs/KNOWN_ISSUES.md instead of writing risky code.
  • Key reasons: update feed is GitHub Releases (objects.githubusercontent.com), whose cert GitHub rotates on its own schedule with no RC control/notice. GitHub has broken pinned clients on this domain before. electron-updater already verifies artifact authenticity via code-signature checks, which covers the real threat (forged/malicious build) regardless of TLS pinning. A stale pin would cause a full, RC-unfixable auto-update outage — blast radius exceeds the residual risk removed.

Test plan

  • Docs-only change, no code touched
  • New entry matches existing KNOWN_ISSUES.md format (Status / Symptom / Root cause / Decision / Affected files)

Summary by CodeRabbit

  • Documentation
    • Added a new known issue entry describing the updater certificate-pinning decision and its resolved risk-acceptance status.
    • Clarified the update feed, signature verification, and operational considerations around certificate rotation.
    • Noted the affected updater configuration and confirmed no product code changes were required.

docs: document CORE-1128 cert pinning risk-acceptance decision
273e60e 
CORE-1128 (2022 pentest finding) flagged missing certificate pinning
for auto-updater connections. Investigated and decided not to
implement it: GitHub Releases is the update feed, GitHub controls
objects.githubusercontent.com's TLS cert rotation, and electron-updater
already verifies downloaded artifacts via code-signature checks. A
pinned cert on a GitHub-managed domain risks a future GitHub-side
rotation causing an unfixable auto-update outage, which outweighs the
residual MITM/CA-compromise risk pinning would mitigate.

This is a documented risk acceptance, not a deferred implementation.

coderabbitai Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info ⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2687940a-6a48-4f92-8654-8665c0568818

📥 Commits

Reviewing files that changed from the base of the PR and between a459f10 and 273e60e.

📒 Files selected for processing (1)
  • docs/KNOWN_ISSUES.md
📜 Recent review details 🧰 Additional context used 🪛 LanguageTool docs/KNOWN_ISSUES.md

[uncategorized] ~47-~47: The official name of this software platform is spelled with a capital “H”.
Context: ...ases (electron-builder.json: provider github, owner RocketChat, repo `Rocket.Ch...

(GITHUB)

[style] ~50-~50: ‘advance notice’ might be wordy. Consider a shorter alternative.
Context: ... rotates on its own schedule, with no advance notice to Rocket.Chat. - Root cause / rational...

(EN_WORDINESS_PREMIUM_ADVANCE_NOTICE)

🔇 Additional comments (1)
docs/KNOWN_ISSUES.md (1)

51-54: 🎯 Functional Correctness

Drop the autoUpdater certificate-hook claim electron-updater exposes the session through autoUpdater.netSession, so this rationale about needing internals is misleading.

			> Likely an incorrect or invalid review comment.

Walkthrough

This change adds a documentation entry to docs/KNOWN_ISSUES.md describing CORE-1128, an accepted risk regarding the absence of certificate pinning for the auto-updater. It explains the rationale and lists electron-builder.json as the affected configuration, with no source code changes.

Changes

Known Issues Documentation

Layer / File(s) Summary
Add CORE-1128 known issue entry
docs/KNOWN_ISSUES.md		 Documents the accepted risk of no certificate pinning for the auto-updater, citing reliance on code-signature verification and referencing electron-builder.json with no code changes.

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

Suggested labels: type: chore

🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the docs-only risk-acceptance decision about not adding certificate pinning.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • CORE-1128: Request failed with status code 401

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

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

jeanfbrito merged commit 3f0b531 into master Jul 9, 2026
10 checks passed
jeanfbrito deleted the security/CORE-1128-cert-pinning-docs branch July 9, 2026 14:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

type: chore

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

Back | FazBrowse Home | New Git URL