Skip to content

fix: HTTPS-only URLs, quieter logs, pipe fail-closed, CI lockstep - #22

Merged
summeroff merged 2 commits into
masterfrom
fix/review-hardening-https-logs-ci
Aug 12, 2026
Merged

summeroff merged 2 commits into
masterfrom
fix/review-hardening-https-logs-ci

Conversation

@summeroff

Copy link
Copy Markdown
Owner

Summary

Hardening from the v1.0.5 architecture/security review (items 1–5). No merge — leave open for Copilot + human.

  1. HTTPS-only URLs

    • IsHttpsUrl on binding url=, default_ai_url, update_url, --ai-url, --update-url, and workflow navigate.
    • Updater uses WINHTTP_OPTION_REDIRECT_POLICY_DISALLOW_HTTPS_TO_HTTP.
    • Companion prepareAndPaste / prepare reject non-https and origins outside manifest.json host_permissions.
  2. Quieter logs

    • Default log_level=info (code + shipped template).
    • Editor capture: length at INFO, preview at DEBUG (same as CONTEXT/payload).
  3. CI lockstep

    • Tag portable zip copies stamped build/RelWithDebInfo/extension (fallback: repo + stamp_extension_version.cmake).
    • Format job installs clang-format-18 and sets CLANG_FORMAT.
  4. Pipe fail-closed

    • Do not create the named pipe if SDDL setup fails.
    • After connect: GetNamedPipeClientProcessId + image name must be this exe (tray dummy connect or --native-messaging-host).
  5. Docs

    • --help and README match shipped hotkeys (J/K/L/I/O/M), AppData config, hidden top-level HWND, https-only URLs.

Existing AppData qiuckprompts.ini is not overwritten. If it still has log_level=debug, change it locally and restart.

Test plan

  • scripts/build.bat Debug compile of all touched .cpp
  • --self-test exit 0 (new cases: default Info, IsHttpsUrl accept/reject, --ai-url http rejected / https accepted)
  • After merge: Reload unpacked companion (background.js origin allowlist)
  • Existing AppData log_level=debug still wins until edited

Reject non-https binding and update URLs; updater denies HTTPS-to-HTTP
redirects. Default log_level=info; editor/clipboard previews stay at
DEBUG. Named pipe fails closed if SDDL setup fails and accepts only
this exe as a client. Tag zip stamps the companion to PE X.Y.Z; CI
pins clang-format 18. --help and README match the shipped product.

Co-authored-by: Hermes/grok-4.6/m3rcur1al <hermes-m3rcur1al@local>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Hardens URL handling, extension IPC, logging defaults, release packaging, and user documentation.

Changes:

  • Enforces HTTPS URLs and extension origin allowlisting.
  • Reduces sensitive INFO logs and secures named-pipe clients.
  • Aligns CI packaging, formatting, help text, and documentation.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/workflow.cpp Validates URLs and quiets capture logs.
src/util.cpp Adds HTTPS URL validation.
src/updater.cpp Prevents insecure feeds and redirects.
src/main.cpp Updates CLI help.
src/ext_bridge.cpp Hardens named-pipe setup and clients.
src/config.cpp Validates configured and CLI URLs.
src/app.cpp Extends self-tests and config guidance.
scripts/format.sh Prefers clang-format 18.
README.md Documents current behavior and paths.
include/util.hpp Declares URL validation API.
include/config.hpp Changes default logging to Info.
extension/background.js Restricts companion navigation origins.
config/qiuckprompts.ini Ships quieter logging defaults.
.github/workflows/ci.yml Pins formatting and stamps packaged extensions.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread include/config.hpp
{
std::wstring logPath;
LogLevel logLevel = LogLevel::Debug;
LogLevel logLevel = LogLevel::Info;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in f5d5bc7 — after LoadConfigFile we re-apply --log-level (CLI wins) then Logger::SetLevel so AppData log_level is actually used.

Comment thread src/util.cpp Outdated
Comment on lines +413 to +414
const std::wstring s = Trim(url);
constexpr wchar_t kPrefix[] = L"https://";

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in f5d5bc7 — IsHttpsUrl now rejects outer whitespace; CLI/INI Trim first and store the trimmed URL.

Comment thread src/util.cpp Outdated
Comment on lines +425 to +428
const wchar_t host0 = s[kPrefixLen];
if (host0 == L'/' || host0 == L'\\' || host0 == L'?' || host0 == L'#')
return false;
return true;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in f5d5bc7 — skip userinfo, require host to start with alnum or [. https://:443/path is rejected; https://[::1]/ still ok.

Comment thread src/ext_bridge.cpp Outdated
Comment on lines +78 to +80
const std::wstring want = FileLeaf(GetExePath());
const std::wstring got = FileLeaf(img);
return !want.empty() && _wcsicmp(want.c_str(), got.c_str()) == 0;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in f5d5bc7 — compare GetFullPathName of this exe vs client image path (case-insensitive). Same-PID dummy connect still allowed.

After LoadConfigFile, re-apply --log-level and SetLevel so AppData
log_level=debug still wins unless CLI overrides. IsHttpsUrl rejects
outer whitespace and empty hosts (https://:443/…). CLI stores the
trimmed URL. Pipe clients must match this exe's full path, not just
the leaf name.

Co-authored-by: Hermes/grok-4.6/m3rcur1al <hermes-m3rcur1al@local>
@summeroff

Copy link
Copy Markdown
Owner Author

Addressed Copilot in f5d5bc7 (4/4):

  1. INI log_level now applied via SetLevel after load; --log-level still wins.
  2. IsHttpsUrl rejects outer whitespace; CLI/INI store the trimmed URL.
  3. Empty host (https://:443/…) rejected; IPv6 [::1] still ok.
  4. Pipe client identity is full path, not leaf name.

Self-test rebuilt green. Left open.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/util.cpp:436

  • The bracket branch only checks the first character, so malformed authorities such as https://[]/ and https://[/ are accepted even though they have no hostname. These values then pass the new config/CLI/workflow guards and fail later during navigation. Validate a non-empty, closed bracketed host (and any following port) before returning true.
    const wchar_t host0 = url[i];
    if (!(iswalnum(host0) || host0 == L'['))
        return false;
    return true;

@summeroff
summeroff merged commit f72e348 into master Aug 12, 2026
9 checks passed
@summeroff
summeroff deleted the fix/review-hardening-https-logs-ci branch August 12, 2026 20:43
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