Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/clipboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ function createNode(text: string): Element {
// Normalize characters that render as a space (or as nothing) so that copied
// text matches what the user sees.
function normalizeText(text: string): string {
return text
.replace(/[\u00A0\u1680\u2000-\u200A\u202F\u205F\u3000]/g, ' ')
.replace(/[\u200B-\u200D\u2060\uFEFF\u180E]/g, '')
return text.replace(/[\u00A0\u1680\u2000-\u200A\u202F\u205F]/g, ' ').replace(/[\u200B-\u200D\u2060\uFEFF\u180E]/g, '')

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.

@jonrohan do you have thoughts on this? 👀 preserving U+3000 fixes the Japanese text issue, but it sounds like it could weaken the command-copy protection from #87/#88. not sure which behavior we want.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Confirmed the parsing difference locally without fetching or executing any remote content: Bash retains U+3000 in the URL argument before the pipe, while an ASCII space terminates that argument. The Japanese text-preservation benefit therefore does not establish that a global exception is appropriate for command-copying callers.

The current PR remains a proposal. I will hold further changes to this exception pending the maintainers' decision on the security tradeoff and whether the intended behavior needs to depend on the caller. This agent-run check is not a substitute for the requested human security decision.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for checking. The parsing difference is real, and I'm not disputing it. I opened #94, so here is the other side of the tradeoff: normalizing U+3000 doesn't remove the mismatch between what users see and what they copy. It moves it.

In Japanese environments, U+3000 commonly appears inside quoted arguments of shell commands:

cp "資料 最終版.xlsx" backup/
grep "サンプル 商品" master.csv
git commit -m "商品名の全角 スペースを修正"

With normalization, the copied command silently targets a different file or searches for a different string than the one displayed. So for command-copying callers too, normalization can make the copied command behave differently from what the user sees, and in this case it happens to legitimate, everyday text rather than to a crafted edge case.

Also, unlike U+00A0, U+3000 is visibly wider than an ASCII space, especially in monospace code blocks, so the spoofing risk is lower than in the case #87 addressed.

For these reasons, I think preserving U+3000, as this PR does, is the safer default overall. If you decide to keep normalization for some callers, I'd ask that U+3000 at least be preserved in non-shell code blocks (CSV, JSON, plain text, etc.). That alone would fix the data-corruption case in #94.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A small correction to my previous comment. English isn't my first language, and I used an AI assistant to help write it. In the process, a compromise ended up in the last paragraph that I don't actually agree with.

Please disregard the suggestion to preserve U+3000 only in non-shell code blocks. As the examples show, shell commands are exactly where silently changing U+3000 causes the most harm, so that compromise is hard for me to accept. My position is that U+3000 should be preserved everywhere, including in shell code blocks, as this PR does.

}

function copySelection(node: Element): Promise<void> {
Expand Down
65 changes: 63 additions & 2 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ describe('clipboard-copy element', function () {
assert.notInclude(text, '\u00A0')
})

it('normalizes Unicode space-separator characters to regular spaces', async function () {
it('normalizes Unicode space-separator characters except ideographic spaces', async function () {
const spaceChars = [
'\u00A0',
'\u1680',
Expand All @@ -196,7 +196,6 @@ describe('clipboard-copy element', function () {
'\u200A',
'\u202F',
'\u205F',
'\u3000',
]

for (const char of spaceChars) {
Expand All @@ -215,6 +214,29 @@ describe('clipboard-copy element', function () {
}
})

it('preserves ideographic spaces in target element text', async function () {
const target = document.createElement('div')
target.textContent = 'product\u3000name'
target.id = 'copy-target'
document.body.append(target)

const button = document.querySelector('clipboard-copy')
button.click()

const text = await whenCopied
assert.equal(text, 'product\u3000name')
})

it('preserves ideographic spaces from the value attribute', async function () {
const button = document.querySelector('clipboard-copy')
button.setAttribute('value', 'product\u3000name')

button.click()

const text = await whenCopied
assert.equal(text, 'product\u3000name')
})

it('strips zero-width and invisible format characters', async function () {
const zeroWidthChars = ['\u200B', '\u200C', '\u200D', '\u2060', '\uFEFF', '\u180E']

Expand All @@ -234,6 +256,45 @@ describe('clipboard-copy element', function () {
}
})

it('preserves ideographic spaces through the legacy fallback', async function () {
const descriptor = Object.getOwnPropertyDescriptor(Navigator.prototype, 'clipboard')
const execCommandDescriptor = Object.getOwnPropertyDescriptor(document, 'execCommand')
assert(descriptor && descriptor.configurable)

delete navigator.clipboard
delete Navigator.prototype.clipboard
assert.equal('clipboard' in navigator, false)

let selectedText
Object.defineProperty(document, 'execCommand', {
configurable: true,
value(command) {
if (command === 'copy') selectedText = getSelection().toString()
return true
},
})

try {
const target = document.createElement('div')
target.textContent = 'product\u3000name'
target.id = 'copy-target'
document.body.append(target)

const button = document.querySelector('clipboard-copy')
button.click()
await whenCopied
} finally {
if (execCommandDescriptor) {
Object.defineProperty(document, 'execCommand', execCommandDescriptor)
} else {
delete document.execCommand
}
Object.defineProperty(Navigator.prototype, 'clipboard', descriptor)
}

assert.equal(selectedText, 'product\u3000name')
})

it('does not copy when disabled', async function () {
const target = document.createElement('div')
target.innerHTML = 'Hello world!'
Expand Down