-
Notifications
You must be signed in to change notification settings - Fork 61
SD-1588 - Added setDocumentProtection command
#1808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
SD-1588 - Added setDocumentProtection command
#1808
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bd621bc25
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| console.log('normalizedMode', normalizedMode); | ||
| console.log('PROTECTION_VALUE_MAP', PROTECTION_VALUE_MAP, normalizedMode in PROTECTION_VALUE_MAP); | ||
| if (!(normalizedMode in PROTECTION_VALUE_MAP)) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reject prototype keys when validating protection modes
The validation uses normalizedMode in PROTECTION_VALUE_MAP, which also accepts prototype properties. If a caller passes a string like "toString" or "__proto__", the check passes even though it is not a supported mode, and mappedValue becomes the inherited function/object. That results in a w:edit attribute set to a non-string value (likely serialized as [object Function]), corrupting settings.xml instead of returning false. Consider using Object.hasOwn/hasOwnProperty to restrict to defined keys.
Useful? React with 👍 / 👎.
caio-pizzol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently looking into STD locks - decided to review this one :)
Nice work @chittolinag
| */ | ||
| function normalizeMode(mode = '') { | ||
| const normalized = typeof mode === 'string' ? mode.trim() : ''; | ||
| console.log('normalized', normalized); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that's debug - remove?
| const mappedValue = PROTECTION_VALUE_MAP[normalizedMode]; | ||
| if (mappedValue) { | ||
| const protectionNode = createDocProtectionNode(mappedValue); | ||
| const insertIndex = existingIndex >= 0 ? existingIndex : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When no existing protection node exists, this inserts at index 0 which puts it before w:zoom.
OOXML spec says documentProtection should come at position 35, after zoom/trackRevisions/etc which might cause schema validation issues.
| * @returns {keyof typeof PROTECTION_VALUE_MAP} | ||
| */ | ||
| function normalizeMode(mode = '') { | ||
| const normalized = typeof mode === 'string' ? mode.trim() : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing null/undefined here becomes empty string, which then falls back to noProtection via the default.
Means invalid input silently removes protection instead of erroring.
Maybe we should return false for non-strings instead?
| }); | ||
|
|
||
| const editor = buildEditor(base); | ||
| const command = setProtectionMode('allowOnlyFormFields'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name says it tests casing but only uses one case
Introduced a new
setProtectionModecommand that lets SuperDoc callers toggle DOCX protection settings directly from the editor. The command normalizes supported modes (no protection, revisions-only, comments-only, form-fields-only, and read-only), patches thew:documentProtectionnode insideword/settings.xml.Examples: