Skip to content

Fix parameter validation and Windows compilation in SaveRingtone security patch - #198

Merged
nijel merged 13 commits into
alert-autofix-25from
copilot/sub-pr-197
Feb 5, 2026
Merged

Fix parameter validation and Windows compilation in SaveRingtone security patch#198
nijel merged 13 commits into
alert-autofix-25from
copilot/sub-pr-197

Conversation

Copilot AI commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

Security fix for unrestricted file permissions (alert #25) had critical bugs: incorrect parameter validation and Windows compilation failures.

Changes

Core Logic Fix

  • Changed from checking value (ringtone dict) to file (filename param) in PyObject_AsFileDescriptor() and PyUnicode_Check()
  • Original code never reached the security-hardened path for string filenames

Windows Platform Support

// Headers - fcntl.h needed on both platforms for O_* constants
#include <sys/stat.h>
#include <fcntl.h>
#ifdef _WIN32
#include <io.h>  // _open(), _close()
#endif

// File creation with owner-only permissions
#ifdef _WIN32
ofd = _open(name, _O_WRONLY | _O_CREAT | _O_TRUNC | _O_BINARY, _S_IREAD | _S_IWRITE);
#else
ofd = open(name, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
#endif

Test Coverage

  • Added test_save_ringtone_permissions validating owner-only file creation (0o600)
  • Tests multiple formats (rttl, ott, imy) with proper ringtone structure
  • Platform-aware: skips permission checks on Windows

Pre-commit

  • Whitelisted WRONLY constant name in typos checker

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits February 5, 2026 09:36
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix file created without restricting permissions Fix Windows compilation and parameter logic in gammu_SaveRingtone Feb 5, 2026
Copilot AI requested a review from nijel February 5, 2026 09:38
@nijel

nijel commented Feb 5, 2026

Copy link
Copy Markdown
Member

@copilot Can you add tests for this to prove the fix is effective? Also fix the pre-commit issue, perhaps by allowing this particular identifier in pyproject.toml.

…ssue

Co-authored-by: nijel <212189+nijel@users.noreply.github.com>

Copilot AI commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Can you add tests for this to prove the fix is effective? Also fix the pre-commit issue, perhaps by allowing this particular identifier in pyproject.toml.

Fixed in commit 8a41e90:

  1. Pre-commit typos issue: Added WRONLY to allowed words in pyproject.toml. This is a legitimate constant name for the write-only flag used in file operations.
  2. Test for file permissions: Added test_save_ringtone_permissions that verifies files are created with owner-only read/write permissions (0o600) on POSIX systems, proving the security fix is effective.

Copilot AI and others added 2 commits February 5, 2026 09:45
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
…le creation

Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
@nijel

nijel commented Feb 5, 2026

Copy link
Copy Markdown
Member

pre-commit.ci autofix

Comment thread test/test_dummy.py Outdated
@nijel

nijel commented Feb 5, 2026

Copy link
Copy Markdown
Member

@copilot Compilation on Windows fails:

gammu/src/gammu.c(4782): warning C4267: 'function': conversion from 'size_t' to 'long', possible loss of data
gammu/src/gammu.c(4858): warning C4267: 'function': conversion from 'size_t' to 'long', possible loss of data
gammu/src/gammu.c(4934): warning C4267: 'function': conversion from 'size_t' to 'long', possible loss of data
gammu/src/gammu.c(6106): error C2065: '_O_WRONLY': undeclared identifier
gammu/src/gammu.c(6106): error C2065: '_O_CREAT': undeclared identifier
gammu/src/gammu.c(6106): error C2065: '_O_TRUNC': undeclared identifier
gammu/src/gammu.c(6106): error C2065: '_O_BINARY': undeclared identifier

Co-authored-by: nijel <212189+nijel@users.noreply.github.com>

Copilot AI commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Compilation on Windows fails:

...

Fixed in commit 083db8d. The issue was that <fcntl.h> was only included for POSIX systems, but Windows also needs it to get the _O_WRONLY, _O_CREAT, _O_TRUNC, and _O_BINARY constants. Now <fcntl.h> is included for both platforms, while <io.h> remains Windows-only for the _open() and _close() functions.

Copilot AI requested a review from nijel February 5, 2026 10:10
@nijel
nijel marked this pull request as ready for review February 5, 2026 10:19
@nijel

nijel commented Feb 5, 2026

Copy link
Copy Markdown
Member

@copilot The test data seems incomplete:

 >           gammu.SaveRingtone(temp_file, ringtone, "rttl")
E           ValueError: Missing key in dictionary: Value

Also, please extend the test to cover all cases the ringtone save is supposed to support.

Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
@nijel

nijel commented Feb 5, 2026

Copy link
Copy Markdown
Member

pre-commit.ci autofix

@nijel

nijel commented Feb 5, 2026

Copy link
Copy Markdown
Member

@copilot There is still and error in the data:

 >           gammu.SaveRingtone(temp_file, ringtone, "rttl")
E           ValueError: Bad value for GSM_RingNoteScale 1

Retry the previous task.

@nijel

nijel commented Feb 5, 2026

Copy link
Copy Markdown
Member

@copilot Retry.

Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
Copilot AI changed the title Fix Windows compilation and parameter logic in gammu_SaveRingtone Fix parameter validation and Windows compilation in SaveRingtone security patch Feb 5, 2026
Comment thread test/test_dummy.py Outdated
@nijel
nijel merged commit 8ca79a9 into alert-autofix-25 Feb 5, 2026
34 checks passed
@nijel
nijel deleted the copilot/sub-pr-197 branch February 5, 2026 12:52
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