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

Protocol Handler: make it possible to set contentType based on content as it arrives #56

Closed
lidel opened this issue Aug 7, 2018 · 3 comments · Fixed by #59
Closed

Comments

@lidel
Copy link
Contributor

lidel commented Aug 7, 2018

Problem

Streaming protocols are unable to set contentType, because current Protocol API (@ 99a15b6) requires contentType to be provided before we start streaming response:

browser.protocol.registerProtocol("dweb", request => {
  return {
    contentType: "text/html",
    content: asyncResponseStreamFor(request.url)
  }
})

This means we need to skip contentType parameter and are required to rely on mime-sniffing present in Firefox itself.

This is far from ideal (svg breakage being a known problem: ipfs-inactive/faq#224 (comment)) and may be also insecure, as there will be use cases where specific array of bytes can render as different things, depending on assumed contentType.

What we want is effectively ability to support X-Content-Type-Options: nosniff scenarios and provide own type without sniffing in browser.

Solution

Protocol handler should be able to set (optional) contentType and contentEncoding based on content it read from internal source, something along this pseudocode:

browser.protocol.registerProtocol("dweb", request => {
  return {
     // contentEncoding: "utf-8"
     // contentType: "text/html",
    content: asyncResponseStreamWithContentType(request.url) 
  }
})

async function asyncResponseStreamWithContentType(url) {
  const response = await responseStream(request.url)
  const {contentType, contentEncoding} = await contentMetadata(response)
  return {
    contentType,
    contentEncoding,
    content: response
  }
}

Basically, we all should be able to buffer a bit of data and extract contentType from it (be it from a static value in content header, or own heuristics for mime-type sniffing) before passing response to the browser.

@Gozala when we discussed this briefly last week, you mentioned it should be possible, but in case more context is needed below are details from IPFS side of things.

Additional Notes on IPFS Use Case

In case of IPFS we should be able to store media type within IPLD structures, as one of "Extended Attributes" in unixfs-v2 (ipld/legacy-unixfs-v2#11) or just mime-sniff it as soon data starts arriving inside of handler itself (with ability to account for edge cases such as SVG).

Below is a snippet with working stream-based mime-type sniffer ported from our Brave PoC:

  const peek = require('buffer-peek-stream')
  // mime-sniff over initial bytes
  const { stream, contentType } = await new Promise((resolve, reject) => {
    peek(ipfs.files.catReadableStream(path), 512, (err, data, stream) => {
      if (err) return reject(err)
      const contentType = mimeSniff(data, path) || 'text/plain'
      resolve({ stream, contentType })
    })
  })
  console.log(`[ipfs-companion] [ipfs://] handler read ${path} and internally mime-sniffed it as ${contentType}`)

  return {stream, contentType}

This is as generic as it gets, I suspect other protocols could also find this ability to set contentType based on beginning of incoming data very useful.

@Gozala
Copy link
Contributor

Gozala commented Aug 8, 2018

Inline is quote from the MDN regarding how nsIChannel (which is what we interact under the hood) handles contentType:

The MIME type of the channel's content if available.

Note: The content type can often be wrongly specified (For example wrong file extension, wrong MIME type, wrong document type stored on a server and so on.), and the caller most likely wants to verify with the actual data.

Setting contentType before the channel has been opened provides a hint to the channel as to what the MIME type is. The channel may ignore this hint in deciding on the actual MIME type that it will report.

Setting contentType after onStartRequest has been fired or after open() is called will override the type determined by the channel.

Setting contentType between the time that asyncOpen() is called and the time when onStartRequest is fired has undefined behavior at this time.

The value of the contentType attribute is a lowercase string. A value assigned to this attribute will be parsed and normalized as follows:

  1. Any parameters (delimited with a ';') will be stripped.
  2. If a charset parameter is given, then its value will replace the the contentCharset attribute of the channel.
  3. The stripped contentType will be lowercased. Any implementation of nsIChannel must follow these rules.

It is also worth pointing out that currently implementation based on asyncOpen and in fact once asyncOpen is invoked we talk to the protocol implementation in extension-process once we receive HEAD / response object without content we update contentCharset, contentLength, contentType if provided and only afterwords we do call onStartRequest which according to MDN has undefined behavior at this time.

So it appears to me that we should set contentType after onStartRequest has being fired but it's unclear to me how many times it could be updated or up until when these updates have an effect.

The reason I'm saying this above is that it could be that turning a handler to an async function may not necessarily map well with how nsIChannel's are consumed, for example proposed API will need to delay streaming content until it can sniff the mime type, although it might make sense to do both in parallel. Or if mime type could be updated multiple times entirely diff API might make more sense.

@Gozala
Copy link
Contributor

Gozala commented Aug 8, 2018

My current plan is to make change API as follows (@lidel I think that is what you meant instead of what you wrote in the description):

// Sync handler
browser.protocol.registerProtocol("dweb", request => {
  return {
     contentEncoding: "utf-8"
     contentType: "text/html",
    content: asyncResponseStreamWithContentType(request.url) 
  }
})


// Async handler
browser.protocol.registerProtocol("dweb", async request => {
  const response = await responseStream(request.url)
  const {contentType, contentEncoding} = await contentMetadata(response)
  return {
    contentType,
    contentEncoding,
    content: response
  }
})

I might have to revisit this decision after I get more insight from networking team on how and when contentType is actually supposed to be set.

Gozala added a commit to Gozala/libdweb that referenced this issue Aug 8, 2018
This allows protocol implementers to sniff content for contentType detection without blocking.

fix mozilla#56
@Gozala Gozala closed this as completed in #59 Aug 8, 2018
@lidel
Copy link
Contributor Author

lidel commented Aug 13, 2018

@Gozala yes, that is even better! (for some reason I assumed registerProtocol can't support async version directly)

Got it working in a local branch and so far everything works as expected, thanks!

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 a pull request may close this issue.

2 participants