vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

fix: ensure same protocol for configured api url #3303

Merged
dpschen merged 4 commits from fix/ensure-same-origin into main 2023-03-26 19:18:49 +00:00
Conversation 2 Commits 4 Files Changed 1 +6 -39
konrad commented 2023-03-24 19:02:00 +00:00
Owner
Copy Link

Resolves https://github.com/go-vikunja/frontend/issues/109

Vikunja would save the api url with http instead of https when the frontend was accessed via https. This was fine in most cases when the server would redirect all requests made to http to the secure https variant. However, in newer Firefox versions (and soon, Chrome probably as well) the browser would not follow that redirect anymore. Hence, we need to make sure to only make api requests to the same protocol. Doing API requests from an https hosted fronted to an http hosted api would probably fail already anyway.

Resolves https://github.com/go-vikunja/frontend/issues/109 Vikunja would save the api url with `http` instead of `https` when the frontend was accessed via https. This was fine in most cases when the server would redirect all requests made to http to the secure https variant. However, in newer Firefox versions (and soon, Chrome probably as well) the browser would not follow that redirect anymore. Hence, we need to make sure to only make api requests to the same protocol. Doing API requests from an https hosted fronted to an http hosted api would probably fail already anyway.
dpschen was assigned by konrad 2023-03-24 19:02:00 +00:00
konrad added 1 commit 2023-03-24 19:02:01 +00:00
konrad requested review from dpschen 2023-03-24 19:03:37 +00:00
frederick commented 2023-03-24 19:08:20 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3303-fix-ensure-same-origin--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3303-fix-ensure-same-origin--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen reviewed 2023-03-24 19:41:33 +00:00
src/helpers/checkAndSetApiUrl.ts Outdated
@ -16,3 +18,3 @@
!url.startsWith('https://')
) {
url = `http://${url}`
if (shouldCheckHttps) {
dpschen commented 2023-03-24 19:41:33 +00:00
Member 
url = shouldCheckHttps
  ? `https://${url}`
  : `http://${url}`
``` url = shouldCheckHttps ? `https://${url}` : `http://${url}` ```
konrad commented 2023-03-24 22:15:58 +00:00
Author
Owner

Even easier:

url = `${window.location.protocol}//${url}`
Even easier: ``` url = `${window.location.protocol}//${url}`
❤️ 1 🚀 1
dpschen marked this conversation as resolved
konrad added 1 commit 2023-03-24 22:16:14 +00:00
continuous-integration/drone/pr Build is passing Details
57aff0a827
fix: simplify appending protocol to api url
dpschen approved these changes 2023-03-24 23:26:18 +00:00
dpschen reviewed 2023-03-25 07:12:01 +00:00
src/helpers/checkAndSetApiUrl.ts Outdated
@ -21,3 +23,4 @@
const urlToCheck: URL = new URL(url)
const origUrlToCheck = urlToCheck
urlToCheck.protocol = shouldCheckHttps ? 'https:' : 'http:'
dpschen commented 2023-03-25 07:12:01 +00:00
Member

This will also be the same as window.location.protocol

This will also be the same as `window.location.protocol`
dpschen commented 2023-03-25 07:14:06 +00:00
Member

because of that the shouldCheckHttps is not necessary anymore.

because of that the `shouldCheckHttps` is not necessary anymore.
konrad commented 2023-03-26 14:35:21 +00:00
Author
Owner

Oh right, I've cleaned it up. Please check again.

Oh right, I've cleaned it up. Please check again.
konrad marked this conversation as resolved
dpschen requested changes 2023-03-25 07:14:38 +00:00
dpschen left a comment
Member

see comment

see comment
konrad added 1 commit 2023-03-26 14:35:06 +00:00
continuous-integration/drone/pr Build is passing Details
a6494ee4f3
fix: simplify protocol further
dpschen reviewed 2023-03-26 16:23:35 +00:00
src/helpers/checkAndSetApiUrl.ts
@ -16,3 +16,3 @@
!url.startsWith('https://')
) {
url = `http://${url}`
url = `${window.location.protocol}//${url}`
dpschen commented 2023-03-26 16:23:34 +00:00
Member

This should already have window.location.protocol since we assign it in line 10.

This should already have `window.location.protocol` since we assign it in line 10.
konrad commented 2023-03-26 16:28:18 +00:00
Author
Owner

In line 10 we only assign the host, not the protocol.

In line 10 we only assign the host, not the protocol.
dpschen commented 2023-03-26 18:01:10 +00:00
Member

oh yes, sry. Hard to read code on mobile

oh yes, sry. Hard to read code on mobile
dpschen commented 2023-03-26 18:01:13 +00:00
Member

oh yes, sry. Hard to read code on mobile

oh yes, sry. Hard to read code on mobile
konrad marked this conversation as resolved
dpschen reviewed 2023-03-26 16:25:34 +00:00
src/helpers/checkAndSetApiUrl.ts Outdated
@ -21,3 +21,4 @@
const urlToCheck: URL = new URL(url)
const origUrlToCheck = urlToCheck
urlToCheck.protocol = window.location.protocol
dpschen commented 2023-03-26 16:25:34 +00:00
Member

since this is based on url it should also already have window.location.protocol

since this is based on `url` it should also already have `window.location.protocol`
konrad commented 2023-03-26 16:29:33 +00:00
Author
Owner

Fixed.

Fixed.
konrad marked this conversation as resolved
konrad added 1 commit 2023-03-26 16:29:26 +00:00
continuous-integration/drone/pr Build is passing Details
d43ad332c9
chore: remove redundant prepending of the protocol
konrad commented 2023-03-26 18:33:29 +00:00
Author
Owner
Copy Link

Can I merge this? (Given how many little commits with one line changes I made and the still tiny size of this PR I think we should squash this)

Can I merge this? (Given how many little commits with one line changes I made and the still tiny size of this PR I think we should squash this)
👍 1
dpschen approved these changes 2023-03-26 19:18:24 +00:00
dpschen merged commit 6c999ad148 into main 2023-03-26 19:18:49 +00:00
dpschen deleted branch fix/ensure-same-origin 2023-03-26 19:18:49 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
dpschen
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#3303
No description provided.
Delete Branch "fix/ensure-same-origin"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?