Skip to content

Address PR #82 feedback: security, documentation, and code clarity#83

Merged
chrisgleissner merged 5 commits into
feat/av-sync-csvfrom
copilot/sub-pr-82
Jan 10, 2026
Merged

Address PR #82 feedback: security, documentation, and code clarity#83
chrisgleissner merged 5 commits into
feat/av-sync-csvfrom
copilot/sub-pr-82

Conversation

Copilot AI commented Jan 10, 2026

Copy link
Copy Markdown
Contributor

Resolves feedback from PR #82 review addressing security vulnerabilities, missing documentation, and code quality issues.

Security: Password exclusion from config files

Passwords are now excluded from both export and import operations to prevent credential leakage via config files:

// Export: password intentionally omitted
fprintf(f, "[network]\n");
fprintf(f, "c64_host=%s\n", c64_host ? c64_host : "");
// Password is intentionally excluded from export for security
fprintf(f, "dns_server_ip=%s\n", dns_server_ip ? dns_server_ip : "");
// Import: password field skipped with info log
} else if (strcmp(key, "c64_password") == 0) {
    // Password is intentionally excluded from import for security.
    // Users must re-enter passwords after importing configuration.
    C64_LOG_INFO("Config import: skipping c64_password (excluded for security)");
}

Passwords remain in OBS settings, protected by OS-level access controls. Users must re-enter passwords after importing configurations.

Documentation: Record A/V Sync feature

Added comprehensive documentation for the "Record A/V Sync" checkbox:

  • Purpose: Creates av-sync.csv with detailed A/V synchronization measurements
  • Automatically triggers A/V sync test program on Ultimate 64 via REST API
  • When to use: Debugging A/V sync issues or contributing test data for bug reports
  • Requirements: Ultimate 64 with REST API enabled (optional, works without for basic CSV recording)

Code quality improvements

  • Added explanatory comments to empty except clauses in av_sync_csv_validation.py (lines 152, 160) clarifying they skip malformed/non-numeric CSV values
  • Removed PLANS.md development artifact from repository

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 4 commits January 10, 2026 22:29
Co-authored-by: chrisgleissner <3969147+chrisgleissner@users.noreply.github.com>
Co-authored-by: chrisgleissner <3969147+chrisgleissner@users.noreply.github.com>
Co-authored-by: chrisgleissner <3969147+chrisgleissner@users.noreply.github.com>
Co-authored-by: chrisgleissner <3969147+chrisgleissner@users.noreply.github.com>
Copilot AI changed the title [WIP] Update PR to address feedback for AV sync test Address PR #82 feedback: security, documentation, and code clarity Jan 10, 2026
Copilot AI requested a review from chrisgleissner January 10, 2026 22:34
@chrisgleissner chrisgleissner marked this pull request as ready for review January 10, 2026 22:59
Copilot AI review requested due to automatic review settings January 10, 2026 22:59
@chrisgleissner chrisgleissner merged commit 2b0a6db into feat/av-sync-csv Jan 10, 2026
16 checks passed
@chrisgleissner chrisgleissner deleted the copilot/sub-pr-82 branch January 10, 2026 23:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses feedback from PR #82 by implementing security improvements, adding documentation, and improving code quality. The changes focus on preventing credential leakage through configuration files while maintaining usability.

Changes:

  • Excluded passwords from config file export/import operations for security
  • Added comprehensive documentation for the "Record A/V Sync" feature
  • Added clarifying comments to empty exception handlers in Python E2E test code
  • Removed development artifact (PLANS.md) from repository

Reviewed changes

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

File Description
src/c64-properties.c Password field excluded from both export and import operations with security comments; import now logs informational message when skipping password
tests/e2e/assertions/av_sync_csv_validation.py Added clarifying comments to empty except clauses explaining they skip malformed CSV values
README.md Updated recording options documentation from 3 to 4 options, added comprehensive "Record A/V Sync" section with purpose, usage, and requirements
PLANS.md Removed 1113-line development planning document from repository

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.

3 participants