{"action":"created","issue":{"url":"https://api.github.com/repos/jgm/CommonMark/issues/270","labels_url":"https://api.github.com/repos/jgm/CommonMark/issues/270/labels{/name}","comments_url":"https://api.github.com/repos/jgm/CommonMark/issues/270/comments","events_url":"https://api.github.com/repos/jgm/CommonMark/issues/270/events","html_url":"https://github.com/jgm/CommonMark/issues/270","id":53494638,"number":270,"title":"Buggy url encoder in commonmark.js","user":{"login":"puzrin","id":319465,"avatar_url":"https://avatars.githubusercontent.com/u/319465?v=3","gravatar_id":"","url":"https://api.github.com/users/puzrin","html_url":"https://github.com/puzrin","followers_url":"https://api.github.com/users/puzrin/followers","following_url":"https://api.github.com/users/puzrin/following{/other_user}","gists_url":"https://api.github.com/users/puzrin/gists{/gist_id}","starred_url":"https://api.github.com/users/puzrin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/puzrin/subscriptions","organizations_url":"https://api.github.com/users/puzrin/orgs","repos_url":"https://api.github.com/users/puzrin/repos","events_url":"https://api.github.com/users/puzrin/events{/privacy}","received_events_url":"https://api.github.com/users/puzrin/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/jgm/CommonMark/labels/JS","name":"JS","color":"d4c5f9"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":19,"created_at":"2015-01-06T09:04:55Z","updated_at":"2015-01-14T18:14:39Z","closed_at":null,"body":"https://github.com/jgm/CommonMark/blob/master/js/lib/inlines.js#L190\r\n\r\nIt can throw exception:\r\n\r\n1. on invalid url format\r\n2. on broken unicode sequence\r\n3. on some valid urls\r\n\r\nAlso it can produce non-standard urls for international domain names.\r\n\r\nI plan migrate to https://github.com/defunctzombie/node-url instead of home-made magical kludges. That can also require to fix some tests content (didn't checked yet).\r\n\r\nNormalization is not mandatory, but it's more simple to use well-tested solution instead of describing all edge cases and writing correct encoder.\r\n\r\nFor reference:\r\n\r\n- https://en.wikipedia.org/wiki/URL_normalization"},"comment":{"url":"https://api.github.com/repos/jgm/CommonMark/issues/comments/69962994","html_url":"https://github.com/jgm/CommonMark/issues/270#issuecomment-69962994","issue_url":"https://api.github.com/repos/jgm/CommonMark/issues/270","id":69962994,"user":{"login":"puzrin","id":319465,"avatar_url":"https://avatars.githubusercontent.com/u/319465?v=3","gravatar_id":"","url":"https://api.github.com/users/puzrin","html_url":"https://github.com/puzrin","followers_url":"https://api.github.com/users/puzrin/followers","following_url":"https://api.github.com/users/puzrin/following{/other_user}","gists_url":"https://api.github.com/users/puzrin/gists{/gist_id}","starred_url":"https://api.github.com/users/puzrin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/puzrin/subscriptions","organizations_url":"https://api.github.com/users/puzrin/orgs","repos_url":"https://api.github.com/users/puzrin/repos","events_url":"https://api.github.com/users/puzrin/events{/privacy}","received_events_url":"https://api.github.com/users/puzrin/received_events","type":"User","site_admin":false},"created_at":"2015-01-14T18:14:39Z","updated_at":"2015-01-14T18:14:39Z","body":"> My question is: does your code guarantee that all URIs that pass through the normalization routine satisfy the RFC?\r\n\r\nYes (except minor things, been done after use `node-url`).\r\n\r\n> Is your opinion that the spec shouldn't say anything about URI normalization? If so, then the spec can't include embedded tests with URIs that aren't already in normalized form.\r\n\r\n1. My opinion is, that if spec says about normalization/encoding, it should cover only \"officially known\" cases (\"valid\" URLs).\r\n2. We need to clarify term \"normalization\", to understand each other right. Issue with broken chars refer to \"URI encoding\". Normalization is replacing entities, converting domain name to lower case and so on. Normalization is optional, encoding is mandatory (for html output).\r\n\r\n> E.g., should the link be to `/%C3%B6` or to `/&ouml;` or to `/ö`?\r\n\r\n(1) and (3) are defined in url encoding rfc. It's enougth to refer. (1) is correct (for HTML). (3) is encodable, if you wish to make 100% correct output. On practice, if you pass it \"as is\", browsers will unterstand it.\r\n\r\n(2) (`/&ouml;`) is not defined anywhere, and belongs to commonmark spec or app level, depending on context. Without context it's impossible to define \"proper\" behaviour.\r\n\r\n> If you're with me so far, then you agree that the spec should say *something* about URI normalization.\r\n\r\nYes of cause. My comments pointed things, that should be avoided there. I meaned, spec should say clear about valid urls, and not say anything about invalid ones.\r\n\r\n> This seems an attractive option to me, especially in light of the following consideration. Suppose you're targeting LaTeX in a renderer. An image might then link to a local file. And in that case, you wouldn't want percent-encoding. So that's an argument that all URI encoding issues should be left up to the renderer, not handled in the parser (which is what the spec covers).\r\n\r\nI think we are specking about different things. URI can be in AST as is - not a problem (and our implementtion don't have AST at all). Though, it's not very clear, that spec says about AST. Also, since many people expect HTML as \"main use\" of markdown it's good to have outputs from different parsers binary equal, when possible."}}