Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions app/controllers/concerns/api/csrf_token_concern.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module Api
module CsrfTokenConcern
def add_csrf_token_header_to_response
response.set_header('X-CSRF-Token', form_authenticity_token)
end
end
end
2 changes: 2 additions & 0 deletions app/controllers/users/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class SessionsController < Devise::SessionsController
include SecureHeadersConcern
include RememberDeviceConcern
include Ial2ProfileConcern
include Api::CsrfTokenConcern

rescue_from ActionController::InvalidAuthenticityToken, with: :redirect_to_signin

Expand All @@ -15,6 +16,7 @@ class SessionsController < Devise::SessionsController
before_action :check_user_needs_redirect, only: [:new]
before_action :apply_secure_headers_override, only: [:new, :create]
before_action :clear_session_bad_password_count_if_window_expired, only: [:create]
after_action :add_csrf_token_header_to_response, only: [:keepalive]

def new
analytics.sign_in_page_visit(
Expand Down
72 changes: 72 additions & 0 deletions app/javascript/packages/request/index.spec.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import sinon from 'sinon';
import type { SinonStub } from 'sinon';
import { useSandbox } from '@18f/identity-test-helpers';
import { request } from '.';

Expand Down Expand Up @@ -25,6 +27,7 @@ describe('request', () => {

expect(window.fetch).to.have.been.calledOnce();
});

it('works even if the CSRF token is not found on the page', async () => {
sandbox.stub(window, 'fetch').callsFake(() =>
Promise.resolve(
Expand All @@ -38,6 +41,7 @@ describe('request', () => {
csrf: () => undefined,
});
});

it('does not try to send a csrf when csrf is false', async () => {
sandbox.stub(window, 'fetch').callsFake((url, init = {}) => {
const headers = init.headers as Headers;
Expand All @@ -54,6 +58,7 @@ describe('request', () => {
csrf: false,
});
});

it('prefers the json prop if both json and body props are provided', async () => {
const preferredData = { prefered: 'data' };
sandbox.stub(window, 'fetch').callsFake((url, init = {}) => {
Expand All @@ -71,6 +76,7 @@ describe('request', () => {
body: JSON.stringify({ bad: 'data' }),
});
});

it('works with the native body prop', async () => {
const preferredData = { this: 'works' };
sandbox.stub(window, 'fetch').callsFake((url, init = {}) => {
Expand All @@ -87,6 +93,7 @@ describe('request', () => {
body: JSON.stringify(preferredData),
});
});

it('includes additional headers supplied in options', async () => {
sandbox.stub(window, 'fetch').callsFake((url, init = {}) => {
const headers = init.headers as Headers;
Expand All @@ -105,6 +112,7 @@ describe('request', () => {
},
});
});

it('skips json serialization when json is a boolean', async () => {
const preferredData = { this: 'works' };
sandbox.stub(window, 'fetch').callsFake((url, init = {}) => {
Expand All @@ -122,6 +130,7 @@ describe('request', () => {
body: JSON.stringify(preferredData),
});
});

it('converts a POJO to a JSON string with supplied via the json property', async () => {
const preferredData = { this: 'works' };
sandbox.stub(window, 'fetch').callsFake((url, init = {}) => {
Expand All @@ -138,4 +147,67 @@ describe('request', () => {
json: preferredData,
});
});

context('with response including csrf token', () => {
beforeEach(() => {
sandbox.stub(window, 'fetch').callsFake(() =>
Promise.resolve(
new Response(JSON.stringify({}), {
status: 200,
headers: [['X-CSRF-Token', 'new-token']],
}),
),
);
});

it('does nothing, gracefully', async () => {
await request('https://example.com', {});
});

context('with global csrf token', () => {
beforeEach(() => {
document.head.innerHTML += `
<meta name="csrf-token" content="token" />
<meta name="csrf-param" content="authenticity_token" />
`;
});

it('replaces global csrf token with the response token', async () => {
await request('https://example.com', {});

const metaToken = document.querySelector<HTMLMetaElement>('meta[name="csrf-token"]')!;
expect(metaToken.content).to.equal('new-token');
});

it('uses response token for next request', async () => {
await request('https://example.com', {});
(window.fetch as SinonStub).resetHistory();
await request('https://example.com', {});
expect(window.fetch).to.have.been.calledWith(
sinon.match.string,
sinon.match((init) => init!.headers!.get('x-csrf-token') === 'new-token'),
);
});

context('with form csrf token', () => {
beforeEach(() => {
document.body.innerHTML += `
<form><input name="authenticity_token" value="token"></form>
<form><input name="authenticity_token" value="token"></form>
`;
});

it('replaces form tokens with the response token', async () => {
await request('https://example.com', {});

const inputs = document.querySelectorAll('input');
expect(inputs).to.have.lengthOf(2);
expect(Array.from(inputs).map((input) => input.value)).to.deep.equal([
'new-token',
'new-token',
]);
});
});
});
});
});
49 changes: 42 additions & 7 deletions app/javascript/packages/request/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,43 @@ interface RequestOptions extends RequestInit {
csrf?: boolean | CSRFGetter;
}

const getCSRFToken = () =>
document.querySelector<HTMLMetaElement>('meta[name="csrf-token"]')?.content;
class CSRF {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not attached to this approach and am open to suggestions, in case there's any concern of it being unclear. I felt it worked okay as a pattern to encapsulate multiple values where some of those values have both getter and setter behaviors attached.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"This" being the Singleton to hold the global values? Seems like a good fit for this case.

One idea could be to have the setters write back to the DOM in case other JS code doesn't use this class? 🤷

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it does use the DOM as the source of truth and sets the new value there, so outside scripts would still have an accurate value if referencing them directly.

if (this.#tokenMetaElement) {
this.#tokenMetaElement.content = value;
}
this.#paramInputElements.forEach((input) => {
input.value = value;
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah whoops I missed that. perfect!

static get token(): string | null {
return this.#tokenMetaElement?.content || null;
}

static set token(value: string | null) {
if (!value) {
return;
}

if (this.#tokenMetaElement) {
this.#tokenMetaElement.content = value;
}

this.#paramInputElements.forEach((input) => {
input.value = value;
});
}

static get param(): string | undefined {
return this.#paramMetaElement?.content;
}

export async function request<Response>(
static get #tokenMetaElement(): HTMLMetaElement | null {
return document.querySelector('meta[name="csrf-token"]');
}

static get #paramMetaElement(): HTMLMetaElement | null {
return document.querySelector('meta[name="csrf-param"]');
}

static get #paramInputElements(): NodeListOf<HTMLInputElement> {
return document.querySelectorAll(`input[name="${this.param}"]`);
}
}

export async function request<Response = any>(
url: string,
options: Partial<RequestOptions> = {},
): Promise<Response> {
Expand All @@ -24,7 +57,7 @@ export async function request<Response>(
headers = new Headers(headers);

if (csrf) {
const csrfToken = typeof csrf === 'boolean' ? getCSRFToken() : csrf();
const csrfToken = typeof csrf === 'boolean' ? CSRF.token : csrf();

if (csrfToken) {
headers.set('X-CSRF-Token', csrfToken);
Expand All @@ -41,9 +74,11 @@ export async function request<Response>(
}

const response = await window.fetch(url, { ...fetchOptions, headers, body });
if (response.ok) {
return json ? response.json() : response.text();
CSRF.token = response.headers.get('X-CSRF-Token');

if (!response.ok) {
throw new Error();
}

throw new Error(await response.json());
return json ? response.json() : response.text();
}
41 changes: 8 additions & 33 deletions app/javascript/packs/session-timeout-ping.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { forceRedirect } from '@18f/identity-url';
import { request } from '@18f/identity-request';
import type { CountdownElement } from '@18f/identity-countdown/countdown-element';
import type { ModalElement } from '@18f/identity-modal';

Expand Down Expand Up @@ -48,17 +49,10 @@ const initialTime = new Date();
const modal = document.querySelector<ModalElement>('lg-modal.session-timeout-modal')!;
const keepaliveEl = document.getElementById('session-keepalive-btn');
const countdownEls: NodeListOf<CountdownElement> = modal.querySelectorAll('lg-countdown');
const csrfEl: HTMLMetaElement | null = document.querySelector('meta[name="csrf-token"]');

let csrfToken = '';
if (csrfEl) {
csrfToken = csrfEl.content;
}

function notifyNewRelic(request, error, actionName) {
function notifyNewRelic(error, actionName) {
(window as LoginGovGlobal).newrelic?.addPageAction('Session Ping Error', {
action_name: actionName,
request_status: request.status,
time_elapsed_ms: new Date().valueOf() - initialTime.valueOf(),
error: error.message,
});
Expand Down Expand Up @@ -100,36 +94,17 @@ function success(data: PingResponse) {
}

function ping() {
const request = new XMLHttpRequest();
request.open('GET', '/active', true);

request.onload = function () {
try {
success(JSON.parse(request.responseText));
} catch (error) {
notifyNewRelic(request, error, 'ping');
}
};
request('/active')
.then(success)
.catch((error) => notifyNewRelic(error, 'ping'));

request.send();
setTimeout(ping, frequency);
}

function keepalive() {
const request = new XMLHttpRequest();
request.open('POST', '/sessions/keepalive', true);
request.setRequestHeader('X-CSRF-Token', csrfToken);

request.onload = function () {
try {
success(JSON.parse(request.responseText));
modal.hide();
} catch (error) {
notifyNewRelic(request, error, 'keepalive');
}
};

request.send();
request('/sessions/keepalive', { method: 'POST' })
.then(success)
.catch((error) => notifyNewRelic(error, 'keepalive'));
}

keepaliveEl?.addEventListener('click', keepalive, false);
Expand Down
19 changes: 19 additions & 0 deletions spec/controllers/concerns/api/csrf_token_concern_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
require 'rails_helper'

RSpec.describe Api::CsrfTokenConcern, type: :controller do
describe '#add_csrf_token_header_to_response' do
controller ApplicationController do
include Api::CsrfTokenConcern

before_action :add_csrf_token_header_to_response

def index; end
end

it 'includes csrf token in the response headers' do
get :index

expect(response.headers['X-CSRF-Token']).to be_kind_of(String)
end
end
end
Loading