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

fix(files): prefer subdomain gw in copied share links #2255

Merged
merged 28 commits into from
Sep 10, 2024

Conversation

acul71
Copy link
Contributor

@acul71 acul71 commented Sep 3, 2024

Issue #2244

When you right click on a file in Files and choose "Share Link" you are given a URL like https://<host>/ipfs/<cid>. This should be of the form https://<cid>.ipfs.<host> for security reasons.

Solution

Modify the setting page and relevant code so that the user can adjust both subdomain and path gateway they use, but by default we use subdomain whenever possible

Code

  • Modified getShareableLink function in src/lib/files.js it now
    Generates a shareable link for the provided files using a subdomain gateway as default or a path gateway as fallback
/**
 * Generates a shareable link for the provided files using a subdomain gateway as default or a path gateway as fallback.
 *
 * @param {FileStat[]} files - An array of file objects with their respective CIDs and names.
 * @param {string} gatewayUrl - The URL of the default IPFS gateway.
 * @param {string} subdomainGatewayUrl - The URL of the subdomain gateway.
 * @param {IPFSService} ipfs - The IPFS service instance for interacting with the IPFS network.
 * @returns {Promise<string>} - A promise that resolves to the shareable link for the provided files.
 */
export async function getShareableLink (files, gatewayUrl, subdomainGatewayUrl, ipfs) {
  let cid
  let filename

  if (files.length === 1) {
    cid = files[0].cid
    if (files[0].type === 'file') {
      filename = `?filename=${encodeURIComponent(files[0].name)}`
    }
  } else {
    cid = await makeCIDFromFiles(files, ipfs)
  }

  const url = new URL(subdomainGatewayUrl)

  /**
   * dweb.link (subdomain isolation) is listed first as the new default option.
   * However, ipfs.io (path gateway fallback) is also listed for CIDs that cannot be represented in a 63-character DNS label.
   * This allows users to customize both the subdomain and path gateway they use, with the subdomain gateway being used by default whenever possible.
   */
  let shareableLink = ''
  const base32Cid = cid.toV1().toString()
  if (base32Cid.length < 64) {
    shareableLink = `${url.protocol}//${base32Cid}.ipfs.${url.host}${filename || ''}`
  } else {
    shareableLink = `${gatewayUrl}/ipfs/${cid}${filename || ''}`
  }

  // console.log('Shareable link', shareableLink)
  return shareableLink
}
  • Added in src/bundles/gateway.js a New function checkSubdomainGateway (gatewayUrl) that Checks if a given gateway URL is functioning correctly by verifying image loading and redirection.
/**
 * Checks if a given URL redirects to a subdomain that starts with a specific hash.
 *
 * @param {URL} url - The URL to check for redirection.
 * @throws {Error} Throws an error if the URL does not redirect to the expected subdomain.
 * @returns {Promise<void>} A promise that resolves if the URL redirects correctly, otherwise it throws an error.
 */
async function expectSubdomainRedirect (url) {
  // Detecting redirects on remote Origins is extra tricky,
  // but we seem to be able to access xhr.responseURL which is enough to see
  // if paths are redirected to subdomains.

  const { redirected, url: responseUrl } = await fetch(url.toString())
  console.log('redirected: ', redirected)
  console.log('responseUrl: ', responseUrl)

  if (redirected) {
    console.log('definitely redirected')
  }
  const { hostname } = new URL(responseUrl)

  if (!hostname.startsWith(IMG_HASH_1PX)) {
    const msg = `Expected ${url.toString()} to redirect to subdomain '${IMG_HASH_1PX}' but instead received '${responseUrl}'`
    console.log(msg)
    throw new Error(msg)
  }
}

/**
 * Checks if an image can be loaded from a given URL within a specified timeout.
 *
 * @param {URL} imgUrl - The URL of the image to be loaded.
 * @returns {Promise<void>} A promise that resolves if the image loads successfully within the timeout, otherwise it rejects with an error.
 */
async function checkViaImgUrl (imgUrl) {
  const imgCheckTimeout = 15000
  await new Promise((resolve, reject) => {
    const img = new Image()
    const timer = setTimeout(() => {
      reject(new Error(`Timeout when attempting to load img from '${img.src}`))
    }, imgCheckTimeout)
    img.onerror = () => {
      clearTimeout(timer)
      reject(new Error(`Error when attempting to load img from '${img.src}`))
    }
    img.onload = () => {
      clearTimeout(timer)
      console.log(`Successfully loaded img from '${img.src}'`)
      resolve(undefined)
    }
    img.src = imgUrl.toString()
  })
}

/**
 * Checks if a given gateway URL is functioning correctly by verifying image loading and redirection.
 *
 * @param {string} gatewayUrl - The URL of the gateway to be checked.
 * @returns {Promise<boolean>} A promise that resolves to true if the gateway is functioning correctly, otherwise false.
 */
export async function checkSubdomainGateway (gatewayUrl) {
  let imgSubdomainUrl
  let imgRedirectedPathUrl
  try {
    const gwUrl = new URL(gatewayUrl)
    imgSubdomainUrl = new URL(`${gwUrl.protocol}//${IMG_HASH_1PX}.ipfs.${gwUrl.hostname}/?now=${Date.now()}&filename=1x1.png#x-ipfs-companion-no-redirect`)
    imgRedirectedPathUrl = new URL(`${gwUrl.protocol}//${gwUrl.hostname}/ipfs/${IMG_HASH_1PX}?now=${Date.now()}&filename=1x1.png#x-ipfs-companion-no-redirect`)
  } catch (err) {
    console.log('Invalid URL:', err)
    return false
  }
  return await checkViaImgUrl(imgSubdomainUrl)
    .then(async () => expectSubdomainRedirect(imgRedirectedPathUrl))
    .then(() => {
      console.log(`Gateway at '${gatewayUrl}' is functioning correctly (verified image loading and redirection)`)
      return true
    })
    .catch((err) => {
      console.log(err)
      return false
    })
}
  • Modified ui setting in settings/SettingsPage.js and labels in public/locales/en/app.json and public/locales/en/settings.json
    image
     <Box className='mb3 pa4-l pa2'>
      <div className='lh-copy charcoal'>
      <Title>{t('app:terms.publicGateway')}</Title>
        <Trans i18nKey='publicSubdomainGatewayDescription' t={t}>
          <p>Choose which <a className='link blue' href="https://docs.ipfs.tech/concepts/ipfs-gateway/#subdomain" target='_blank' rel='noopener noreferrer'>Subdomain Gateway</a> you want to use when generating shareable links (Default: Subdomain Isolation)</p>
        </Trans>
        <PublicSubdomainGatewayForm/>
      </div>
      <div className='lh-copy charcoal'>
        <Trans i18nKey='publicPathGatewayDescription' t={t}>
          <p>Choose which <a className='link blue' href="https://docs.ipfs.tech/concepts/ipfs-gateway/#path" target='_blank' rel='noopener noreferrer'>Path Gateway</a>  you want to use when generating shareable links (Fallback: path gateway used for CIDs that can't be represented in 63 character DNS label)</p>
        </Trans>
        <PublicGatewayForm/>
      </div>
    </Box>

##############

Tests: settings page
I've added a couple of tests in test/e2e/settings.test.js for submit/reset functionality of public gateway

test('Submit/Reset Public Subdomain Gateway', async ({ page }) => {
    // Wait for the necessary elements to be available in the DOM
    const publicSubdomainGatewayElement = await page.waitForSelector('#public-subdomain-gateway')
    const publicSubdomainGatewaySubmitButton = await page.waitForSelector('#public-subdomain-gateway-submit-button')
    const publicSubdomainGatewayResetButton = await page.waitForSelector('#public-subdomain-gateway-reset-button')

    // Check that submitting a wrong Subdomain Gateway triggers a red outline
    await submitGatewayAndCheck(page, publicSubdomainGatewayElement, publicSubdomainGatewaySubmitButton, DEFAULT_PATH_GATEWAY, 'focus-outline-red')

    // Check that submitting a correct Subdomain Gateway triggers a green outline
    await submitGatewayAndCheck(page, publicSubdomainGatewayElement, publicSubdomainGatewaySubmitButton, DEFAULT_SUBDOMAIN_GATEWAY + '/', 'focus-outline-green')

    // Check the Reset button functionality
    await resetGatewayAndCheck(publicSubdomainGatewayResetButton, publicSubdomainGatewayElement, DEFAULT_SUBDOMAIN_GATEWAY)
  })

  test('Submit/Reset Public Path Gateway', async ({ page }) => {
    // Custom timeout for this specific test
    test.setTimeout(32000)

    // Wait for the necessary elements to be available in the DOM
    const publicGatewayElement = await page.waitForSelector('#public-gateway')
    const publicGatewaySubmitButton = await page.waitForSelector('#public-path-gateway-submit-button')
    const publicGatewayResetButton = await page.waitForSelector('#public-path-gateway-reset-button')

    // Check that submitting a wrong Path Gateway triggers a red outline
    await submitGatewayAndCheck(page, publicGatewayElement, publicGatewaySubmitButton, DEFAULT_PATH_GATEWAY + '1999', 'focus-outline-red')

    // Check that submitting a correct Path Gateway triggers a green outline
    await submitGatewayAndCheck(page, publicGatewayElement, publicGatewaySubmitButton, DEFAULT_SUBDOMAIN_GATEWAY, 'focus-outline-green')

    // Check the Reset button functionality
    await resetGatewayAndCheck(publicGatewayResetButton, publicGatewayElement, DEFAULT_PATH_GATEWAY)
  })

And another test src/lib/files.test.js for checking subdomain and path gateway urls

it('should get a subdomain gateway url', async () => {
  const ipfs = {}
  const myCID = CID.parse('QmZTR5bcpQD7cFgTorqxZDYaew1Wqgfbd2ud9QqGPAkK2V')
  const file = {
    cid: myCID,
    name: 'example.txt'
    // type: 'file',
  }
  const files = [file]

  const url = new URL(DEFAULT_SUBDOMAIN_GATEWAY)
  const shareableLink = await getShareableLink(files, DEFAULT_PATH_GATEWAY, DEFAULT_SUBDOMAIN_GATEWAY, ipfs)
  const base32Cid = 'bafybeifffq3aeaymxejo37sn5fyaf7nn7hkfmzwdxyjculx3lw4tyhk7uy'
  const rightShareableLink = `${url.protocol}//${base32Cid}.ipfs.${url.host}`
  // expect(res).toBe('https://bafybeifffq3aeaymxejo37sn5fyaf7nn7hkfmzwdxyjculx3lw4tyhk7uy.ipfs.dweb.link')
  expect(shareableLink).toBe(rightShareableLink)
})

it('should get a path gateway url', async () => {
  const ipfs = {}
  // very long CID v1 (using sha3-512)
  const veryLongCidv1 = 'bagaaifcavabu6fzheerrmtxbbwv7jjhc3kaldmm7lbnvfopyrthcvod4m6ygpj3unrcggkzhvcwv5wnhc5ufkgzlsji7agnmofovc2g4a3ui7ja'
  const myCID = CID.parse(veryLongCidv1)
  const file = {
    cid: myCID,
    name: 'example.txt'
  }
  const files = [file]

  const res = await getShareableLink(files, DEFAULT_PATH_GATEWAY, DEFAULT_SUBDOMAIN_GATEWAY, ipfs)
  // expect(res).toBe('https://ipfs.io/ipfs/bagaaifcavabu6fzheerrmtxbbwv7jjhc3kaldmm7lbnvfopyrthcvod4m6ygpj3unrcggkzhvcwv5wnhc5ufkgzlsji7agnmofovc2g4a3ui7ja')
  expect(res).toBe(DEFAULT_PATH_GATEWAY + '/ipfs/' + veryLongCidv1)
})
 git show --stat
commit cdde8884d22d8683afa97e2b0959287fd6769463 (HEAD -> fix-issue-2244, origin/fix-issue-2244)
Author: acul71 
Date:   Tue Sep 3 02:09:47 2024 +0200

    Fix issue #2244: Updated public gateway settings, added subdomain gateway support, and refactored tests.

 public/locales/en/app.json                                                 |   3 +++
 public/locales/en/settings.json                                            |   3 ++-
 src/bundles/files/actions.js                                               |   4 ++-
 src/bundles/gateway.js                                                     | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 src/components/public-gateway-form/PublicGatewayForm.js                    |  10 ++++---
 src/components/public-subdomain-gateway-form/PublicSubdomainGatewayForm.js |  94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/lib/files.js                                                           |  31 ++++++++++++++++-----
 src/lib/files.test.js                                                      |  38 +++++++++++++++++++++++++-
 src/settings/SettingsPage.js                                               |  23 ++++++++++------
 test/e2e/settings.test.js                                                  |  83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 10 files changed, 374 insertions(+), 26 deletions(-)

acul71 and others added 2 commits September 3, 2024 02:09
Fix issue ipfs#2244 Share Link incorrectly gives path routed instead of subdomain routed URL
@acul71 acul71 requested a review from a team as a code owner September 3, 2024 17:39
Copy link

welcome bot commented Sep 3, 2024

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

@lidel lidel changed the title Fix issue 2244 fix(files): prefer subdomain gw when generating share link Sep 3, 2024
@lidel lidel changed the title fix(files): prefer subdomain gw when generating share link fix(files): prefer subdomain gw in copied share links Sep 3, 2024
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you @acul71, this is very useful improvement that allows users to avoid hitting HTTP redirect on page load.

Did a quick first-pass review, small asks below.

src/bundles/gateway.js Outdated Show resolved Hide resolved
src/bundles/gateway.js Outdated Show resolved Hide resolved
src/bundles/gateway.js Outdated Show resolved Hide resolved
src/settings/SettingsPage.js Outdated Show resolved Hide resolved
src/settings/SettingsPage.js Outdated Show resolved Hide resolved
@lidel lidel mentioned this pull request Sep 3, 2024
34 tasks
acul71 and others added 5 commits September 4, 2024 16:27
raw block without dag-pb envelope, saves us 8 bytes

Co-authored-by: Marcin Rataj <[email protected]>
reuse const

Co-authored-by: Marcin Rataj <[email protected]>
this is GUI app, let's keep it short and avoid technical jargon

Co-authored-by: Marcin Rataj <[email protected]>
prefer simpler, single sentence.

Co-authored-by: Marcin Rataj <[email protected]>
…s, refactored image check code, and simplified settings text.
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

a few nits and one change that I think will simplify the new form/input a bit (removing showFailState)

src/bundles/gateway.js Outdated Show resolved Hide resolved
src/bundles/gateway.js Outdated Show resolved Hide resolved
src/bundles/gateway.js Outdated Show resolved Hide resolved
src/bundles/gateway.js Outdated Show resolved Hide resolved
src/lib/files.js Outdated Show resolved Hide resolved
acul71 and others added 9 commits September 4, 2024 19:45
remove debug code

Co-authored-by: Russell Dempsey <[email protected]>
Co-authored-by: Russell Dempsey <[email protected]>
Co-authored-by: Russell Dempsey <[email protected]>
Co-authored-by: Russell Dempsey <[email protected]>
Co-authored-by: Russell Dempsey <[email protected]>
Co-authored-by: Russell Dempsey <[email protected]>
Co-authored-by: Russell Dempsey <[email protected]>
Co-authored-by: Russell Dempsey <[email protected]>
@acul71 acul71 requested a review from SgtPooki September 4, 2024 18:49
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

lgtm, two minor changes requested. will wait to merge for @lidel to take a peek

src/lib/files.test.js Outdated Show resolved Hide resolved
src/bundles/gateway.js Outdated Show resolved Hide resolved
acul71 and others added 2 commits September 5, 2024 13:58
Co-authored-by: Russell Dempsey <[email protected]>
Co-authored-by: Russell Dempsey <[email protected]>
@acul71
Copy link
Contributor Author

acul71 commented Sep 6, 2024

@lidel
Hello, I requested a review is the code ok? Thanks, Bye

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

lgtm, works locally..

we do not need to probe default dweb.link every time,
doing it for custom gateways is enough
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you @acul71!

Pushed small fix in 44a43be to stop spamming console + avoid hitting dweb.link by all users who run with default gateway.

@lidel lidel merged commit e8c4421 into ipfs:main Sep 10, 2024
22 checks passed
@acul71 acul71 deleted the fix-issue-2244 branch September 11, 2024 10:57
ipfs-gui-bot pushed a commit that referenced this pull request Sep 23, 2024
## [4.3.1](v4.3.0...v4.3.1) (2024-09-23)

 CID `bafybeideglc722hiwhsy4kiyl2fivf5lr6wozy2iuixtgzkvl3v4hasaty`

 ---

### Bug Fixes

* **file-preview:** safeSubresourceGwUrl ([#2253](#2253)) ([bb861a3](bb861a3)), closes [/github.com//issues/2246#issuecomment-2322192398](https://github.com/ipfs//github.com/ipfs/ipfs-webui/issues/2246/issues/issuecomment-2322192398)
* **files:** avoid duplicated fetch during preview ([#2254](#2254)) ([eefae25](eefae25)), closes [#2217](#2217)
* **files:** prefer subdomain gw in copied share links ([#2255](#2255)) ([e8c4421](e8c4421))

### Trivial Changes

* .io → .tech ([b9f622d](b9f622d))
* browserslist@latest ([#2251](#2251)) ([809c55a](809c55a))
* Pull transifex translations ([#2258](#2258)) ([2250374](2250374))
* use [email protected] ([#2257](#2257)) ([99ba9ac](99ba9ac))
@ipfs-gui-bot
Copy link
Collaborator

🎉 This PR is included in version 4.3.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Share Link incorrectly gives path routed instead of subdomain routed URL.
4 participants