Skip to content
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

PDF content blocked by Chrome #916

Closed
rgaudin opened this issue Mar 15, 2023 · 68 comments · Fixed by #924 or #930
Closed

PDF content blocked by Chrome #916

rgaudin opened this issue Mar 15, 2023 · 68 comments · Fixed by #924 or #930
Assignees
Milestone

Comments

@rgaudin
Copy link
Member

rgaudin commented Mar 15, 2023

When opening a PDF in a ZIM in the kiwix-serve viewer, on Chrome, there is an unrecoverable error message. This looks like a regression.

Screenshot 2023-03-15 at 14 18 57

Go to https://dev.library.kiwix.org/viewer#lilote_fr_fo_2023-03/xay-va-%C3%A0-la-p%C3%AAche then click the Lire l'histoire button.

@rgaudin
Copy link
Member Author

rgaudin commented Mar 15, 2023

Might be related to #604

@mgautierfr
Copy link
Member

I have no problem with firefox, chrome and chromium.

@rgaudin
Copy link
Member Author

rgaudin commented Mar 15, 2023

@Popolechien reported it ; I can reproduce on latest Chrome

@rgaudin
Copy link
Member Author

rgaudin commented Mar 15, 2023

Sorry the link is incorrect ; you have to use the viewer. I need to open a ticket about that as well

@rgaudin
Copy link
Member Author

rgaudin commented Mar 15, 2023

@mgautierfr
Copy link
Member

mgautierfr commented Mar 15, 2023

Indeed. Removing the sandbox attribute solves the problem.
This is probably introduced by #906 (issue kiwix/kiwix-tools#604)

@Jaifroid
Copy link
Member

Jaifroid commented Mar 15, 2023

The code I give in that issue works around this specific problem with the pdf viewer in Chrome/Edge. It is not recognised by the browser as same origin, hence PDFs must be opened in a new window or tab. kiwix/kiwix-tools#604 (comment)

@rgaudin
Copy link
Member Author

rgaudin commented Mar 15, 2023

That's quite a consequence in term of UI. If it's not possible to render it in the same iframe, maybe we should use an in-zim pdf.js but that's not as convenient

@Jaifroid
Copy link
Member

Personally I think this is a Chromium bug, because the in-browser PDF viewer should clearly be same-origin when loading a PDF from the same origin. Or maybe it's a feature because PDFs can have active content that can contact external servers (?).

I didn't find any other workaround in Kiwix JS than to make click on a PDF open a new window/tab. Having said that, the new window uses the same in-built PDF viewer, so it's not too big a deal. Epubs have to be downloaded anyway because no browser provides a custom viewer.

It's a balance between the security of the iframe (in terms of not leaking info out: a particular concern with Zimit archives) and (in)convenience... Otherwise, there's really nothing to stop a script in the iframe navigating elsewhere.

NB sandboxing the iframe doesn't stop a determined malicious attacker, but it stops accidental redirects, accidental (or well intentioned) attempts by scripts to break out of iframes, and accidental contacting of external sites for font files, images and (potentially) scripts.

@mgautierfr
Copy link
Member

See whatwg/html#3958 which give some information.

PDF viewer is implemented as a plugin in chrome and it is deactivated in sandboxed iframe.

@kelson42 kelson42 added this to the 12.1.0 milestone Mar 15, 2023
@danielzgtg
Copy link

Please also add allow-downloads. Some people may have a Firefox setup where "Settings > General > Files and Applications > Applications > Portable Document Format (PDF)" is set to "Save File" instead of "Open in Firefox". I heard there was a recent proposal to use ZIMs for serving .apk files, and this would be necessary to support their use case.

The sandbox attribute needs to stay to stop the Wiktionary zim from breaking. What's wrong with pdf.js? It would only make the experience more consistent across platforms.

@veloman-yunkan
Copy link
Collaborator

I guess that a fix addressing #912 but not this ticket doesn't make much sense. On the other hand, I think that a fix to this issue should also automatically fix #912.

@Jaifroid
Copy link
Member

This is what we went for in Kiwix JS (for the sandbox attribute of iframe, or can be served as part of CSP response header):

allow-same-origin allow-scripts allow-modals allow-forms allow-popups allow-downloads

@Jaifroid
Copy link
Member

@veloman-yunkan Is this sandbox attribute actually in force at library.kiwix.org yet? Because if you go to https://library.kiwix.org/viewer#wiktionary_en_all_nopic_2023-02 , you clearly see that a top-level navigation occurs and the iframe is destroyed..., and if I inspect the iframe just before the offending script runs that breaks out of it, I see what's in the screenshot below.

Now, if the sandbox isn't implemented at library.kiwix.org, it means that the issues with the PDF viewer are not directly down to #906, unless I'm missing something (or my browser cache is VERY persistent, despite clearing it)...

(Though I do think #906 will block PDFs in Chrome, because I already had to patch that in Kiwix JS.) Confused 😖...

image

@Jaifroid
Copy link
Member

And, btw, that referrerpolicy should probably be none, rather than same-origin.

@veloman-yunkan
Copy link
Collaborator

@Jaifroid https://library.kiwix.org runs the latest release of kiwix-serve. #906 has not been released yet. Its side effects are currently demonstrated at https://dev.library.kiwix.org (to which this ticket refers).

@Jaifroid
Copy link
Member

@veloman-yunkan Thank you for clearing that up!

@veloman-yunkan
Copy link
Collaborator

If it's not possible to render it in the same iframe, maybe we should use an in-zim pdf.js but that's not as convenient

@rgaudin Another option is to embed pdf.js in kwix-serve. However, I wonder what kind of inconvenience you referred to and whether it applies to the proposed solution too.

@Jaifroid
Copy link
Member

Personally, I'd say there's no real inconvenience in the simplest solution of opening the PDF in a new tab or window. In many ways, it's better UX, because the user can keep that tab to read later and carry on browsing in the iframe. It also parallels the experience with EPUBs, which download separately rather than opening in the iframe. But I get that people have different opinions about such things!

@rgaudin
Copy link
Member Author

rgaudin commented Mar 20, 2023

Bundling pdf.js inside a ZIM means displaying PDF ourselves, via pdf.js. It means creating a host webpage which won't feel exactly as the in-browser pdf.js. Also, for those with other PDF reader configured, it means rendering in PDF.js then potentially downloading (pdf.js allows this) to trigger the default PDF reader.
It's far from being as comfortable as the normal situation.

As for including pdf.js in kiwix-serve, I don't see how that would help… Do you want to add a kiwix-serve specific API to use it? Do you want to intercept application/pdf responses and render them as text/html with pdf.js ?

Then you have the SW discussion all over again (reader-ZIM dependency that's not part of the spec)

@rgaudin
Copy link
Member Author

rgaudin commented Mar 20, 2023

But I get that people have different opinions about such things!

Exactly.

We can look at this from both angles though:

  • ZIM creator decides how he wants his content to be used.
  • Reader creator has control over its reader UX and doesn't have to follow Web conventions.

The trick is that kiwix-serve is both a first-class reader and a Web party so boundaries are blurred.

I don't care much about the outcome of the tab thing but I am worried that we may be starting to drop support for regular web features. @kelson42 @Popolechien what do you think?

@Jaifroid
Copy link
Member

For anyone coming to this longish thread at this point, the executive summary is as follows:

  • There is a vulnerability in Kiwix Serve and Kiwix JS related to the use of iframes to display articles: scripts in these articles can accidentally (or on purpose) navigate to remote sites and leak user data to remote servers, or they can break out of the iframe and destroy the app's controls;
  • A sandbox has therefore been added to the iframe to plug this vulnerability. This sandbox will not stop a purposefully malicious ZIM, but it will stop accidental navigation (as happens with recent Wiktionary), or accidental leakage (as happens when downloading remote fonts or, occasionally, images);
  • However, adding this sandbox proves to have a side effect that in Chromium browsers, the built-in PDF viewer is untrusted (by design, because PDFs can have active content), and PDFs are therefore blocked from rendering in a sandboxed iframe;
  • It is proposed instead that if a user clicks on a PDF link, the PDF will be opened in a new tab or window (not subject to sandboxing);
  • This is not the behaviour the creator of the ZIM may have intended, so there is a balance to be struck between security and respect for the intentions of the ZIM creator / trusted web resource.

@mgautierfr
Copy link
Member

There is a vulnerability in Kiwix Serve and Kiwix JS related to the use of iframes to display articles: scripts in these articles can accidentally (or on purpose) navigate to remote sites and leak user data to remote servers, or they can break out of the iframe and destroy the app's controls;

No. The vulnerability is that we display a "falsely offline" version of website which can still phone home and leak user data to remote servers. This is how web works and except by blocking all connections to server, it can always append.

Before iframe, kiwix-serve was simply displaying the content in the top frame. "falsely offline" websites were obviously able to phone home. And as we were inserting the top bar in the content of the page, it was even simpler for the website to break out the app's controls.
The iframe is the occasion to add some kind of protection against accidentally "falsely offline" website when it was impossible to do before. The change to iframe is not the source of the leak.
However, we have exchange the website css breaking the css of the app controls against links with target=_top breaking the app controls.

There is no more user data leak than before.
You can use the "no viewier/iframe" with the /content/zim_name/path/to/article to see that website can phone home all the time.

Blocking this requests was never a goal of kiwix-serve. It may change, but if it became a purpose, the solution is probably more in service worker (to control all connection going out of the displayed website) than in iframe.

This is not the behaviour the creator of the ZIM may have intended, so there is a balance to be struck between security and respect for the intentions of the ZIM creator / trusted web resource.

There is a balance between adding a new security level and not inferring in the website content (in chrome browser)


Blocking any link with target=_top (with sandboxed iframe) may not be a good idea neither.
A website may be internally composed of several iframes. On iframe may display a menu with links changing the top iframe content. Allowing target=_top link breaks the (viewer) ui, but at least the website is usable.
And this is difficulty patchable as other player than kiwix-serve/kiwix-js don't use iframe and so target=_top is totally valid.
It would be to the viewer itself to dynamically patch (or intercept) links with target=_top and replace them with target=framename.

However, if the target=_top is not really needed, we can consider that as a bug in the scrappers and they should remove it.

@Jaifroid
Copy link
Member

Jaifroid commented Mar 20, 2023

@mgautierfr in kiwix/kiwix-tools#604 (comment) I suggested that instead of using the sandbox attribute on the iframe, Kiwix Serve can serve all content with a CSP sandbox response header. I agree with you that this would be conceptually more elegant. However, we would still have this issue with PDF content not being rendered in Chromium browsers in the iframe, and also the issue with external links being blocked. Like in Kiwix Android, these will still have to be intercepted and opened in an external window. Adding the sandbox attribute in the response header or adding it in the iframe are exactly equivalent in terms of functionality (I've tested this in the PWA).

EDIT: It might be possible to serve only HTML with a CSP sandbox response header, but not serve PDFs with this header. It would need testing as to whether this would allow them to render in the iframe.

@danielzgtg
Copy link

No. The vulnerability

There are multiple versions of the bug. There is more information on Slack.

website css breaking the css of the app controls

Shadow DOM was designed for this problem.

a balance between adding a new security level and not inferring in the website content (in chrome browser)

And between fixing the website content (in Wiktionary)

A website may be internally composed of several iframes

Have we seen how archive.org does it? archive.org edits the HTML of all webpages to insert its header with the controls. It rewrites links, and this is to ensure multimedia paths work and external links are intercepted. With that strategy, we can either use the CSP HTTP header like you suggested, or we can inject a <meta> tag. archive.org does not edit the data of multimedia such as images, so PDFs should be rendered properly if we switch to that strategy.

@Jaifroid
Copy link
Member

@danielzgtg I used to use only a <meta> tag CSP in Kiwix JS PWA, but unfortunately you can't use the sandbox attribute in <meta http-equiv...>. See Content-Security-Policy/sandbox:

This directive is not supported in the <meta> element or by the Content-Security-policy-Report-Only header field.

And without sandbox we can't stop top-level navigation. The other disadvantage of CSPs via <meta> is that you have to alter the HTML to inject the tag, whereas adding a CSP response header or iframe sandbox does not require altering the HTML in any way.

Rewriting links isn't necessary if you intercept the user's click on the iframe and inspect the target.

@kelson42
Copy link
Collaborator

Thx for investogating this in details, but pretty possible that a bug in Chrome has been fixed between versions 90 and 110.

@Jaifroid
Copy link
Member

Yes, I've just checked on Browser Stack, and I confirm that the bug is present in Chrome 90, but it was fixed in Chrome 91 (pictured below) and later. Chrome 90 doesn't recognize the PDF loaded as being of origin 'self'.

image

@rgaudin
Copy link
Member Author

rgaudin commented Mar 28, 2023

FYI Chrome 91 was introduced in May 2021

@Jaifroid
Copy link
Member

Yes, it's pretty recent, but it's actually quite hard to stay on Chrome 90, because Google updates it almost as soon as you install it. Of course that wouldn't be the case with Chromium on Linux.

I haven't spent a long time trying to find out if there's a workaround for Chrome 90, but I did try specifying the site (https://kiwix.github.io) as an allowed frame-src and even tried deleting the mtea http-equiv CSP, but the bug persists. The error in DevTools just confirms the block.

image


Bottom line, unless someone has a specific patch for Chrome <=90's behaviour, it seems to me we have these choices:

  1. Revert sandboxing added in A pseudosafe iframe #906, leaving the Kiwix Serve iframe vulnerable to top-level navigation like in the February Wiktionary (which started all this!) and XSS;
  2. Adopt the fix we're testing here (needs further careful testing with different ZIMs), but advise users of Chrome <= 90 that they will need to upgrade if they want to view PDFs;
  3. Add the workaround which opens any PDF in a separate tab or window (sample code provided here: Add sandbox attribute to the iframe generated by Kiwix Serve to prevent top-level navigation breaking out of the iframe (or malicious scripts) kiwix-tools#604 (comment)). NB workaround is only for PDFs opened with a user gesture.

In Kiwix JS we decided to adopt 3 (actually version 2 + 3), as it is the most universal solution, even if it doesn't mimic precisely what the original web site developers may have intended. Users are forced to open EPUBs in a separate app, so opening PDFs in a separate tab is not so much of a sacrifice IMHO.

@kelson42
Copy link
Collaborator

Can we just move forward the fix, even if chrome90- will continue to fail because of the bug? These users will still have the freedom to read the PDF content via a third party app... we might even trigger this behaviour via http headers based on rhe user-agent I guess.

@rgaudin
Copy link
Member Author

rgaudin commented Mar 29, 2023

Yes, it's pretty recent, but it's actually quite hard to stay on Chrome 90, because Google updates it almost as soon as you install it.

Only applicable to Desktop users with Online access

These users will still have the freedom to read the PDF content via a third party app... we might even trigger this behaviour via http headers based on rhe user-agent I guess.

We can only trigger a download via Content-Disposition header IMO and that's far from being a good solution, especially on mobile where this would just trigger some subtle UI animation/notification.

Yet I also believe we should go forward with this. There's no good solution anyway and we have no data regarding the User Agents of our users (especially offline ones). The only data we can extrapolate from is known deployments which clearly are Android Chrome <90… but on Android, Chrome doesn't have a built-in PDF reader so it's less of a concern.

@Jaifroid
Copy link
Member

Jaifroid commented Mar 29, 2023

UPDATE: The problem with Chrome <= 90 should now be resolved in test version 3.7.5 at https://kiwix.github.io/kiwix-js/ (wait for it to notify that an update is available). To test loading PDFs in the iframe, be sure to turn off the option "Open external links and PDFs in new tabs" (under Display Settings).

Explanation: I realized that the Service Worker was setting the sandbox response headers for PDF files as well as HTML files, so I added the code below exempting PDFs. Something similar could be done in the back-end code of Kiwix Serve. The outcome, in Chrome 90 running in Win11 on Browser Stack, is in the screenshots at bottom.

// Set Content-Security-Policy to sandbox the content (prevent XSS attacks from malicious ZIMs), but not if we're dealing with a PDF, as Chromium <= 90
// has a bug that prevents PDFs from loading if CSP is set
if (!(contentType === 'application/pdf' || /\.pdf$/i.test(msgPortEvent.data.title))) {
    headers.set('Content-Security-Policy', "default-src 'self' data: blob: about: chrome-extension: https://moz-extension.kiwix.org https://kiwix.github.io 'unsafe-inline' 'unsafe-eval'; sandbox allow-scripts allow-same-origin allow-modals allow-popups allow-forms allow-downloads;");
    headers.set('Referrer-Policy', 'no-referrer');
}
if (contentType) headers.set('Content-Type', contentType);

To be clear, this exemption does open a security hole for PDFs, which would no longer be sandboxed, but I think we have to live with that unless we are prepared to force opening PDFs in a new tab or window, or make it an option as in this test version of Kiwix JS. Kiwix Serve doesn't have anywhere obvious to put such options, and I suspect it would complicate the UI code too much to have to deal with such things (options would need to be stored in cookies, read and restored on launch, etc.).

image
image

@veloman-yunkan
Copy link
Collaborator

Explanation: I realized that the Service Worker was setting the sandbox response headers for PDF files as well as HTML files, so I added the code below exempting PDFs. Something similar could be done in the back-end code of Kiwix Serve.

@Jaifroid That was already the case in my local modification to kiwix-serve. I also added the kiwix-serve host address (http://localhost:8080) to default-src in both Content-Security-Policy header and the meta http-equiv CSP, but it didn't help. Can the fact that the content is served over http rather than https be a problem?

@veloman-yunkan
Copy link
Collaborator

My changes (without the temporary hack of hardcoding http://localhost:8080 in default-src) can be seen in #924

@Jaifroid
Copy link
Member

That was already the case in my local modification to kiwix-serve. I also added the kiwix-serve host address (http://localhost:8080) to default-src in both Content-Security-Policy header and the meta http-equiv CSP, but it didn't help. Can the fact that the content is served over http rather than https be a problem?

@veloman-yunkan Localhost is always http but it is considered secure (for testing purposes) by browsers, though I guess we can't rule out a bug here in different browsers. I see you removed the sandbox on the iframe, so it is really puzzling we're getting different results for different implementations. Are all caches fully cleared when you're testing? Might just be worth checking in dev tools that there is no sandbox attribute still set on the iframe... Otherwise, I think we'd need a test implementation to debug what's going on.

@veloman-yunkan
Copy link
Collaborator

I see you removed the sandbox on the iframe, so it is really puzzling we're getting different results for different implementations. Are all caches fully cleared when you're testing? Might just be worth checking in dev tools that there is no sandbox attribute still set on the iframe...

Just checked in dev tools that there is no sandbox attribute set on the iframe.

Localhost is always http but it is considered secure (for testing purposes) by browsers

That's nice but we need kiwix-serve to also work in a LAN over http. So does your answer imply that it may be a problem?

@Jaifroid
Copy link
Member

Jaifroid commented Mar 29, 2023

That's nice but we need kiwix-serve to also work in a LAN over http. So does your answer imply that it may be a problem?

I don't think we're requiring secure origins as part of the sandbox, but we'd need to check the documentation to see if that is enforced or not. Or else test it empirically. However, I can definitively say that v3.7.5 of Kiwix JS works exactly the same whether served from localhost or served from https://kiwix.github.io, so I don't think this has anything to do with the reason we are seeing differences between Kiwix JS and Kiwix Serve despite both trying to serve the same headers.

One unrelated issue with serving ZIM content over LAN using http is that Zimit archives, which depend on a Service Worker (requiring a secure origin) won't work. Zimit archives do work over http://localhost, however, as this is considered secure.

EDIT: I can't see anything in https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/sandbox that presumes a secure origin.

@danielzgtg
Copy link

It is the one I outlined above, i.e. that we serve articles from the ZIM with a CSP sandbox response header, but not PDFs

Thank you for this alternative fix. I confirmed that my Wiktionary use case still works on 3.7.5.

Only applicable to Desktop users with Online access

Only applicable if we don't accept my suggestion to just bundle the Chrome/Firefox installers for Windows/macOS/Linux/Android

security hole for PDFs, which would no longer be sandboxed

Content-Type should already prevent any PDF from being run as HTML/JS. And unless we bundle pdf.js, we can only assume that Chrome sandboxs PDFs to not run Javascript and to not make network requests

I can't see anything in https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/sandbox that presumes a secure origin.

See https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts

It is possible to add a custom domain to /etc/hosts if you don't have two computers to test with

@rgaudin
Copy link
Member Author

rgaudin commented Mar 30, 2023

Only applicable if we don't accept my suggestion to just bundle the Chrome/Firefox installers for Windows/macOS/Linux/Android

This is hardly realistic to expect users to download and install and use a browser ; at least in all the deployment scenarios I've seen. Not to mention the potential system version conflict that might exist. I doubt latest Chrome can be installed on Android 5.1

Should it be done, it should be external to kiwix-serve anyway. We do offer some apps to download. It would be instructive to test with a browser though.

@rgaudin
Copy link
Member Author

rgaudin commented Apr 4, 2023

It just came to my attention that we have a somewhat similar issue with EPUB with firefox

First, verify the kiwix-serve 3.4.0 behavior by going to library. Click the HTML button of any book, it works. Go back and click an EPUB link, it should either open in your in-browser epub extension if you have one or trigger a download.

Now, using kiwix-serve nightly, go to dev-library

Reproduce those steps and you'll notice that nothing happens when you click on the EPUB link. This is even worst UX than the PDF behavior because there is no feedback and the user would only assume that the ZIM is broken.

You can right-click and open in a new tab, and it would work as expected.

As you'd imagine, the console mentions the culprit:

Download of “https://dev.library.kiwix.org/content/gutenberg_en_all_2023-03/Romeo%20and%20Juliet.1513.epub” was blocked because the triggering iframe has the sandbox flag set.

Same behavior with Chrome:

Download is disallowed. The frame initiating or instantiating the download is sandboxed, but the flag ‘allow-downloads’ is not set. See https://www.chromestatus.com/feature/5706745674465280 for more details.

@Jaifroid
Copy link
Member

Jaifroid commented Apr 5, 2023

The code I suggested explicitly includes "allow-downloads", but it's currently only in a PR #924, and not yet in the dev version. It would be useful if @veloman-yunkan could merge that code so we can thoroughly test and debug it in the dev server for issues such as this.

@Jaifroid Jaifroid reopened this Apr 9, 2023
@Jaifroid
Copy link
Member

Jaifroid commented Apr 9, 2023

I'm re-opening because it's not fixed. @veloman-yunkan, the main difference I can see, at first sight, between the implementation at https://kiwix.github.io/kiwix-js/ and the latest Kiwix Serve at https://dev.library.kiwix.org/ is that in the latter case the sandbox is applied to the top-level document, i.e. to the viewer document (see screenshot). In the Kiwix JS version, I removed the top-level sandbox, so that it could be applied on a per-document basis to the content in the iframe. Setting it at top level probably means it can't be turned off when a PDF is being loaded in the iframe. Could you try removing the sandbox on viewer (but leave it for any content other than PDFs loaded from the ZIM)?

image

@rgaudin
Copy link
Member Author

rgaudin commented Apr 9, 2023

@Jaifroid please elaborate on how it's not fixed. @kelson42 and I tested the use cases in the ticket and both were resolved

@Jaifroid
Copy link
Member

Jaifroid commented Apr 9, 2023

@rgaudin Assuming the code in https://dev.library.kiwix.org, after the merging of #924, is the same code you have been testing, then as shown in the screenshot above, PDFs do not load in the iframe in Chromium browsers, whereas in the test case at https://kiwix.github.io/kiwix-js/, they do. @veloman-yunkan had also said that it was not working. I believe the reason may be because the HTML document that contains the iframe is sandboxed in the dev version of Kiwix Serve, whereas it is not sandboxed in the Kiwix JS test case. The sandbox response header should only be served for content from the ZIM that is injected into the iframe (with PDFs excluded).

@kelson42
Copy link
Collaborator

@Jaifroid What are the reproduction steps? unclear for me as well.

@rgaudin
Copy link
Member Author

rgaudin commented Apr 10, 2023

Indeed, we tested both issues with Firefox while the first one is Chrome-only.

@veloman-yunkan
Copy link
Collaborator

@Jaifroid Thanks for testing and identifying the difference. In my "fix" (#924) I incorrectly apply the "Content-Security-Policy" header to all non-PDF responses. I will now add that header only to content intended to be displayed in the viewer (served via the /content endpoint).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment