Skip to content

[XFA] Send URLs as strings, rather than objects (issue 1773) #13417

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 22, 2021
Merged

[XFA] Send URLs as strings, rather than objects (issue 1773) #13417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0dba468
Don't allow the `LoopbackPort` to "clone" a `URL`
Snuffleupagus May 22, 2021
ba13bd8
[XFA] Send `URL`s as strings, rather than objects (issue 1773)
Snuffleupagus May 22, 2021
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
2 changes: 1 addition & 1 deletion src/core/xfa/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2257,7 +2257,7 @@ class Image extends StringObject {
};

if (this.href) {
html.attributes.src = new URL(this.href);
html.attributes.src = new URL(this.href).href;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that for now we shouldn't create an image in case of this.href !== null:

  • the ref can be a path (a name) to an internal picture which is not in the xml stream and we'll handle this case
  • it can be a real http:// link or whatever. In this case, what's the policy ? especially if the url has the file protocol.
  • it can be garbage as you saw it in the mentioned pdf.
    And I've the same question for a tag in xhtml.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main objective of the PR was simply to avoid this line completely breaking everything, because of structured cloning errors (since a URL can't be cloned), and further improvements can probably happen elsewhere?

  • it can be a real http:// link or whatever. In this case, what's the policy ? especially if the url has the file protocol.

I'm not sure that we should even attempt to support that case at all, given the possibly security implications of loading arbitrary content; this feels a bit similar to e.g. #13266 (comment).

return html;
}

Expand Down
5 changes: 4 additions & 1 deletion src/display/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1800,7 +1800,10 @@ class LoopbackPort {
}
return result;
}
result = Array.isArray(value) ? [] : {};
if (value instanceof URL) {
throw new Error(`LoopbackPort.postMessage - cannot clone: ${value}`);
}
result = Array.isArray(value) ? [] : Object.create(null);
cloned.set(value, result); // Adding to cache now for cyclic references.
// Cloning all value and object properties, however ignoring properties
// defined via getter.
Expand Down