-
Notifications
You must be signed in to change notification settings - Fork 324
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
fix(rules): Better Redirect Rules #1256
Changes from 14 commits
7f68ab4
d94b9bb
a817045
045e660
f6561b9
c020638
fc085b5
007f41f
f18579b
3e72d36
d36a282
6ee2d31
e592755
5c85d84
fca5fe2
832679d
5e22cea
dbc672c
381f63c
4020796
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,16 +1,21 @@ | ||||
import debug from 'debug' | ||||
import browser from 'webextension-polyfill' | ||||
import { CompanionState } from '../../types/companion.js' | ||||
import isIPFS from 'is-ipfs' | ||||
|
||||
// this won't work in webworker context. Needs to be enabled manually | ||||
// https://github.com/debug-js/debug/issues/916 | ||||
const log = debug('ipfs-companion:redirect-handler:blockOrObserve') | ||||
log.error = debug('ipfs-companion:redirect-handler:blockOrObserve:error') | ||||
|
||||
const DEFAULT_NAMESPACES = new Set(['ipfs', 'ipns']) | ||||
|
||||
export const GLOBAL_STATE_CHANGE = 'GLOBAL_STATE_CHANGE' | ||||
export const GLOBAL_STATE_OPTION_CHANGE = 'GLOBAL_STATE_OPTION_CHANGE' | ||||
export const DELETE_RULE_REQUEST = 'DELETE_RULE_REQUEST' | ||||
export const DELETE_RULE_REQUEST_SUCCESS = 'DELETE_RULE_REQUEST_SUCCESS' | ||||
|
||||
// We need to match the rest of the URL, so we can use a wildcard. | ||||
export const RULE_REGEX_ENDING = '((?:[^\\.]|$).*)$' | ||||
|
||||
interface regexFilterMap { | ||||
|
@@ -108,6 +113,16 @@ function escapeURLRegex (str: string): string { | |||
return str.replace(ALLOWED_CHARS_URL_REGEX, '\\$1') | ||||
} | ||||
|
||||
/** | ||||
* Compute the namespace from the URL. | ||||
* | ||||
* @param url string | ||||
*/ | ||||
function computeNamespaceFromUrl (url: string): string { | ||||
const { pathname } = new URL(url) | ||||
return (/\/([^/]+)\//i.exec(pathname)?.[1] ?? '').toLowerCase() | ||||
} | ||||
|
||||
/** | ||||
* Construct a regex filter and substitution for a redirect. | ||||
* | ||||
|
@@ -119,6 +134,84 @@ function constructRegexFilter ({ originUrl, redirectUrl }: redirectHandlerInput) | |||
regexSubstitution: string | ||||
regexFilter: string | ||||
} { | ||||
let regexSubstitution = redirectUrl | ||||
let regexFilter = originUrl | ||||
const originURL = new URL(originUrl) | ||||
const redirectNS = computeNamespaceFromUrl(redirectUrl) | ||||
const originNS = computeNamespaceFromUrl(originUrl) | ||||
if (!DEFAULT_NAMESPACES.has(originNS) && DEFAULT_NAMESPACES.has(redirectNS)) { | ||||
// A redirect like https://github.com/ipfs/ipfs-companion/issues/1255 | ||||
regexFilter = `^${escapeURLRegex(regexFilter)}`.replace(/https?/ig, 'https?') | ||||
const origRegexFilter = regexFilter | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of the code in this function is hard to follow, and I doubt someone else will be able to maintain it easily. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in #1261 |
||||
|
||||
const [tld, root, ...subdomain] = originURL.hostname.split('.').reverse() | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For example:
A safer way is to do what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lidel yes, it does the same, I am just removing the known parts, maybe Your example:
The regex you shared fails on e.g.: https://bafybeib3bzis4mejzsnzsb65od3rnv5ffit7vsllratddjkgfgq4wiamqu.on.fleek.co/ because it does not havd PS: Also, I like |
||||
const staticUrl = [root, tld] | ||||
while (subdomain.length > 0) { | ||||
const subdomainPart = subdomain.shift() | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe Or, A potentially more type-safe approach could be to do something like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in #1261 |
||||
const commonStaticUrlStart = `^${originURL.protocol}\\:\\/\\/` | ||||
const commonStaticUrlEnd = `\\.${escapeURLRegex(staticUrl.join('.'))}\\/${RULE_REGEX_ENDING}` | ||||
if (isIPFS.cid(subdomainPart as string)) { | ||||
// We didn't find a namespace, but we found a CID | ||||
// e.g. https://bafybeib3bzis4mejzsnzsb65od3rnv5ffit7vsllratddjkgfgq4wiamqu.on.fleek.co | ||||
regexFilter = `${commonStaticUrlStart}(.*?)${commonStaticUrlEnd}` | ||||
regexSubstitution = redirectUrl | ||||
.replace(subdomainPart as string, '\\1') // replace CID | ||||
.replace(new RegExp(`${originURL.pathname}?$`), '\\2') // replace path | ||||
|
||||
break | ||||
} | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 Unsure if i got this right, but iirc this naive catch-all is not something we've supported before, and may lead to false-positives if someone has non-ipfs domain that happens to be a valid CID. Domains like My initial suggestion would be to remove this (we don't want to keep list of "blessed" services, and services should follow specs if they want to get protocol upgrade: subdomain convention, or There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the concern, fleek for example is one of those services, we only add this rule if the old code evaluates this redirect i.e. origin url and destination url are modified. We can discuss that as a follow-up issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment #1253 (comment) clarifies the issue bit more, I'll remove this check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, if we dont know the namespace, we can't make informed decision about the intention behind the CID, so better to not introduce "guesswork". |
||||
if (DEFAULT_NAMESPACES.has(subdomainPart as string)) { | ||||
// We found a namespace, this is going to match group 2, i.e. namespace. | ||||
// e.g https://bafybeib3bzis4mejzsnzsb65od3rnv5ffit7vsllratddjkgfgq4wiamqu.ipfs.dweb.link | ||||
regexFilter = `${commonStaticUrlStart}(.*?)\\.(${[...DEFAULT_NAMESPACES].join('|')})${commonStaticUrlEnd}` | ||||
|
||||
regexSubstitution = redirectUrl | ||||
.replace(subdomain.reverse().join('.'), '\\1') // replace subdomain or CID. | ||||
.replace(`/${subdomainPart as string}/`, '/\\2/') // replace namespace dynamically. | ||||
|
||||
const pathWithSearch = originURL.pathname + originURL.search | ||||
if (pathWithSearch !== '/') { | ||||
regexSubstitution = regexSubstitution.replace(pathWithSearch, '/\\3') // replace path | ||||
} else { | ||||
regexSubstitution += '\\3' | ||||
} | ||||
|
||||
break | ||||
} | ||||
// till we find a namespace or CID, we keep adding subdomains to the staticUrl. | ||||
staticUrl.unshift(subdomainPart as string) | ||||
} | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we split this big while chunk out into another function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed: #1261 |
||||
|
||||
if (regexFilter !== origRegexFilter) { | ||||
// we found a valid regexFilter, so we can return. | ||||
return { regexSubstitution, regexFilter } | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm unsure how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in #1261 |
||||
} else { | ||||
// we didn't find a valid regexFilter, so we can return the default. | ||||
regexFilter = originUrl | ||||
} | ||||
} | ||||
|
||||
// if the namespaces are the same, we can generate simpler regex. | ||||
// The only value that needs special handling is the `uri` param. | ||||
// TODO: Remove this check, as `uri` param is deprecated. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 this is new to me – where can I read more about this deprecation? or is it about deprecation of use in companion for something (if so, needs clarification/rephrasing) (afaik There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am sorry about this, this comment is irrelevant (only the comment, the code is still good) The reason I got that impression was: shows me: After companion redirects it correctly to: http://bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi.ipfs.localhost:8080/?argTest#hashTest I see the correct will remove this comment, apologies and good catch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, interesting edge case. The 422 error on
|
||||
if ( | ||||
DEFAULT_NAMESPACES.has(originNS) && | ||||
DEFAULT_NAMESPACES.has(redirectNS) && | ||||
originNS === redirectNS && | ||||
originURL.searchParams.get('uri') == null | ||||
) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we name this check? what does the aggregate of all these statements mean? i.e. const hasSameRedirectAndOriginNamespace = hasSameNamespace(originURL, originNS, redirectNS) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in #1261 |
||||
// A redirect like | ||||
// https://ipfs.io/ipfs/QmZMxU -> http://localhost:8080/ipfs/QmZMxU | ||||
const [originFirst, originLast] = originUrl.split(`/${originNS}/`) | ||||
const defaultNSRegexStr = `(${[...DEFAULT_NAMESPACES].join('|')})` | ||||
regexFilter = `^${escapeURLRegex(originFirst)}\\/${defaultNSRegexStr}\\/${RULE_REGEX_ENDING}` | ||||
.replace(/https?/ig, 'https?') | ||||
regexSubstitution = redirectUrl | ||||
.replace(`/${redirectNS}/`, '/\\1/') | ||||
.replace(originLast, '\\2') | ||||
return { regexSubstitution, regexFilter } | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be great to encapsulate the different return values throughout this function so we could name them and break up this function a bit more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in #1261 |
||||
} | ||||
|
||||
// We can traverse the URL from the end, and find the first character that is different. | ||||
let commonIdx = 1 | ||||
while (commonIdx < Math.min(originUrl.length, redirectUrl.length)) { | ||||
|
@@ -129,12 +222,10 @@ function constructRegexFilter ({ originUrl, redirectUrl }: redirectHandlerInput) | |||
} | ||||
|
||||
// We can now construct the regex filter and substitution. | ||||
let regexSubstitution = redirectUrl.slice(0, redirectUrl.length - commonIdx + 1) + '\\1' | ||||
regexSubstitution = redirectUrl.slice(0, redirectUrl.length - commonIdx + 1) + '\\1' | ||||
// We need to escape the characters that are allowed in the URL, but not in the regex. | ||||
const regexFilterFirst = escapeURLRegex(originUrl.slice(0, originUrl.length - commonIdx + 1)) | ||||
// We need to match the rest of the URL, so we can use a wildcard. | ||||
const RULE_REGEX_ENDING = '((?:[^\\.]|$).*)$' | ||||
let regexFilter = `^${regexFilterFirst}${RULE_REGEX_ENDING}`.replace(/https?/ig, 'https?') | ||||
regexFilter = `^${regexFilterFirst}${RULE_REGEX_ENDING}`.replace(/https?/ig, 'https?') | ||||
|
||||
// This method does not parse: | ||||
// originUrl: "https://awesome.ipfs.io/" | ||||
|
@@ -247,11 +338,11 @@ async function reconcileRulesAndRemoveOld (state: CompanionState): Promise<void> | |||
} | ||||
} | ||||
|
||||
const { port } = new URL(state.gwURLString) | ||||
// make sure that the default rules are added. | ||||
for (const { originUrl, redirectUrl } of DEFAULT_LOCAL_RULES) { | ||||
const { port } = new URL(state.gwURLString) | ||||
const regexFilter = `^${escapeURLRegex(`${originUrl}:${port}`)}(.*)$` | ||||
const regexSubstitution = `${redirectUrl}:${port}\\1` | ||||
const regexFilter = `^${escapeURLRegex(`${originUrl}:${port}`)}${RULE_REGEX_ENDING}` | ||||
const regexSubstitution = `${redirectUrl}:${port}/\\1` | ||||
|
||||
if (!savedRegexFilters.has(regexFilter)) { | ||||
// We need to add the new rule. | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
declare module 'is-ipfs' { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created ipfs-shipyard/is-ipfs#88 |
||
function cid (value: string): boolean | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we describe a little more what this is doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment describing it.