diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 970a7a86b9..d0e30889ae 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -81,6 +81,15 @@ repos: contentcuration/kolibri_public/migrations/0004_auto_20240612_1847.py| contentcuration/kolibri_public/migrations/0006_auto_20250417_1516.py| )$ + # Only checks the root Makefile. Extend if nested Makefiles get added. + - repo: local + hooks: + - id: makefile-syntax + name: Makefile syntax check + entry: make -n + language: system + files: ^Makefile$ + pass_filenames: false # Always keep black as the final hook so it reformats any other reformatting. - repo: https://github.com/python/black rev: 20.8b1 diff --git a/Makefile b/Makefile index 292efc9c99..27c222a7d2 100644 --- a/Makefile +++ b/Makefile @@ -39,7 +39,7 @@ migrate: # 4) Remove the management command from this `deploy-migrate` recipe # 5) Repeat! deploy-migrate: - python contentcuration/manage.py ensure_versioned_databases_exist & python contentcuration/manage.py create_channel_versions & wait + echo "Nothing to do here!" contentnodegc: python contentcuration/manage.py garbage_collect diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/EditorToolbar.vue b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/EditorToolbar.vue index 3749f7640f..41e6f73add 100644 --- a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/EditorToolbar.vue +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/EditorToolbar.vue @@ -386,10 +386,14 @@ // Categories that can overflow (in order of overflow priority) const OVERFLOW_CATEGORIES = [ 'insert', - 'script', + // Perseus flavoured markdown does not support super and sub script, + // so we disable this for now until we stop using markdown as the primary target + // 'script', 'lists', 'clearFormat', - 'align', + // Perseus flavoured markdown does not support alignment, + // so we disable this for now until we stop using markdown as the primary target + // 'align', 'clipboard', 'textFormat', ]; diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/toolbar/MobileFormattingBar.vue b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/toolbar/MobileFormattingBar.vue index 2d254fa4a9..7cd939cf70 100644 --- a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/toolbar/MobileFormattingBar.vue +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/toolbar/MobileFormattingBar.vue @@ -81,13 +81,17 @@ :is-active="action.isActive" @click="action.handler" /> + + transformPastedHTML(html), }, onCreate: () => { isReady.value = true; diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/composables/useToolbarActions.js b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/composables/useToolbarActions.js index 9b77b3deaa..27215c97bd 100644 --- a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/composables/useToolbarActions.js +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/composables/useToolbarActions.js @@ -1,6 +1,6 @@ import { computed, inject } from 'vue'; import { getTipTapEditorStrings } from '../TipTapEditorStrings'; -import { sanitizePastedHTML } from '../utils/markdown'; +import { transformPastedHTML } from '../utils/pasteTransform'; export function useToolbarActions(emit) { const editor = inject('editor', null); @@ -165,7 +165,7 @@ export function useToolbarActions(emit) { if (item.types.includes('text/html')) { const htmlBlob = await item.getType('text/html'); const html = await htmlBlob.text(); - const cleaned = sanitizePastedHTML(html); + const cleaned = transformPastedHTML(html); editor.value.chain().focus().insertContent(cleaned).run(); return; diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/markdown.js b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/markdown.js index 90fd88adcc..a364942e39 100644 --- a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/markdown.js +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/markdown.js @@ -39,6 +39,11 @@ export const paramsToImageMd = ({ src, alt, width, height, permanentSrc, textAli return `![${alt || ''}](${IMAGE_PLACEHOLDER}/${fileName}${alignSuffix})`; }; +// --- Underline Translation --- +// Perseus simple-markdown treats __text__ as ; marked treats it as . +// Rewrite before marked runs so the round-trip preserves underline. +export const UNDERLINE_REGEX = /__([\s\S]+?)__(?!_)/g; + // --- Math/Formula Translation --- export const MATH_REGEX = /\$\$([^$]+)\$\$/g; @@ -53,67 +58,6 @@ export const paramsToMathMd = ({ latex }) => { return `$$${latex || ''}$$`; }; -export function sanitizePastedHTML(html) { - if (!html) return ''; - // This code ine 55 to 66 is geneted with the help of LLM with the prompt - // "Create a function that sanitizes HTML pasted from Microsoft - // Word by removing Word-specific tags, styles, and classes while preserving other formatting." - let cleaned = html; - cleaned = cleaned.replace(//gis, ''); - cleaned = cleaned.replace(/<\/?(w|m|o|v):[^>]*>/gis, ''); - const parser = new DOMParser(); - const doc = parser.parseFromString(cleaned, 'text/html'); - doc.querySelectorAll('*').forEach(el => { - if (el.hasAttribute('style')) { - const style = el.getAttribute('style') || ''; - const filtered = style - .split(';') - .map(s => s.trim()) - .filter(s => s && !s.toLowerCase().startsWith('mso-')) - .join('; '); - if (filtered) { - el.setAttribute('style', filtered); - } else { - el.removeAttribute('style'); - } - } - if (el.hasAttribute('class')) { - const cls = el - .getAttribute('class') - .split(/\s+/) - .filter(c => c && !/^Mso/i.test(c)) - .join(' '); - if (cls) { - el.setAttribute('class', cls); - } else { - el.removeAttribute('class'); - } - } - }); - const strikeElements = doc.querySelectorAll('s, strike, del'); - strikeElements.forEach(el => { - const nestedLists = el.querySelectorAll('ul, ol'); - if (nestedLists.length > 0) { - nestedLists.forEach(list => { - el.parentNode.insertBefore(list, el.nextSibling); - }); - } - }); - const lists = doc.querySelectorAll('ul, ol'); - lists.forEach(list => { - const items = list.querySelectorAll(':scope > li'); - items.forEach(item => { - const nestedLists = Array.from(item.children).filter( - child => child.tagName === 'UL' || child.tagName === 'OL', - ); - nestedLists.forEach(nestedList => { - item.appendChild(nestedList); - }); - }); - }); - return doc.body.innerHTML; -} - /** * Pre-processes a raw Markdown string to convert custom syntax into HTML tags * that Tiptap's extensions can understand. This is our custom "loader". @@ -151,5 +95,7 @@ export function preprocessMarkdown(markdown) { return ``; }); + processedMarkdown = processedMarkdown.replace(UNDERLINE_REGEX, '$1'); + return marked(processedMarkdown); } diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/markdownSerializer.js b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/markdownSerializer.js index 38f331b377..5892701658 100644 --- a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/markdownSerializer.js +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/markdownSerializer.js @@ -24,7 +24,7 @@ export const createCustomMarkdownSerializer = editor => { trimmedText = `*${trimmedText}*`; break; case 'underline': - trimmedText = `${trimmedText}`; + trimmedText = `__${trimmedText}__`; break; case 'strike': trimmedText = `~~${trimmedText}~~`; diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/pasteTransform.js b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/pasteTransform.js new file mode 100644 index 0000000000..8f805aa0e9 --- /dev/null +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/utils/pasteTransform.js @@ -0,0 +1,83 @@ +function stripMsoConditionalComments(html) { + return html.replace(//gis, ''); +} + +function stripOfficeNamespacedTags(html) { + return html.replace(/<\/?(w|m|o|v):[^>]*>/gis, ''); +} + +function filterMsoStyleDeclarations(doc) { + doc.querySelectorAll('[style]').forEach(el => { + const filtered = el + .getAttribute('style') + .split(';') + .map(s => s.trim()) + .filter(s => s && !s.toLowerCase().startsWith('mso-')) + .join('; '); + if (filtered) { + el.setAttribute('style', filtered); + } else { + el.removeAttribute('style'); + } + }); +} + +function filterMsoClasses(doc) { + doc.querySelectorAll('[class]').forEach(el => { + const cls = el + .getAttribute('class') + .split(/\s+/) + .filter(c => c && !/^Mso/i.test(c)) + .join(' '); + if (cls) { + el.setAttribute('class', cls); + } else { + el.removeAttribute('class'); + } + }); +} + +function hoistListsOutOfStrike(doc) { + doc.querySelectorAll('s, strike, del').forEach(el => { + el.querySelectorAll('ul, ol').forEach(list => { + el.parentNode.insertBefore(list, el.nextSibling); + }); + }); +} + +function reparentNestedListsInLi(doc) { + doc.querySelectorAll('ul, ol').forEach(list => { + list.querySelectorAll(':scope > li').forEach(item => { + Array.from(item.children) + .filter(child => child.tagName === 'UL' || child.tagName === 'OL') + .forEach(nestedList => item.appendChild(nestedList)); + }); + }); +} + +function stripImages(doc) { + doc.querySelectorAll('img').forEach(el => el.remove()); +} + +const STRING_TRANSFORMS = [stripMsoConditionalComments, stripOfficeNamespacedTags]; + +const DOM_TRANSFORMS = [ + filterMsoStyleDeclarations, + filterMsoClasses, + hoistListsOutOfStrike, + reparentNestedListsInLi, + stripImages, +]; + +export function transformPastedHTML(html) { + if (!html) return ''; + let cleaned = html; + for (const transform of STRING_TRANSFORMS) { + cleaned = transform(cleaned); + } + const doc = new DOMParser().parseFromString(cleaned, 'text/html'); + for (const transform of DOM_TRANSFORMS) { + transform(doc); + } + return doc.body.innerHTML; +} diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/markdown.spec.js b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/markdown.spec.js index 5c5944e86e..8afd770293 100644 --- a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/markdown.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/markdown.spec.js @@ -70,7 +70,28 @@ describe('preprocessMarkdown', () => { }); }); - // 4: Standard Markdown Passthrough + // 4: Perseus Underline Handling + // Perseus's simple-markdown treats __text__ as , but `marked` (CommonMark) + // treats it as . We must rewrite __text__ -> text before marked + // sees it, so the round-trip preserves underline instead of turning it into bold. + describe('Perseus Underline Handling', () => { + it('should rewrite __text__ to text before passing to marked', () => { + preprocessMarkdown('This is __underlined__ text.'); + expect(marked).toHaveBeenCalledWith('This is underlined text.'); + }); + + it('should rewrite multiple __ runs on the same line independently', () => { + preprocessMarkdown('__one__ and __two__'); + expect(marked).toHaveBeenCalledWith('one and two'); + }); + + it('should rewrite __ spanning multiple words', () => { + preprocessMarkdown('__multi word underline__'); + expect(marked).toHaveBeenCalledWith('multi word underline'); + }); + }); + + // 5: Standard Markdown Passthrough describe('Standard Markdown Passthrough', () => { it('should pass non-custom syntax through to the marked library', () => { const standardMd = 'Here is **bold** and a [link](url).'; diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/markdownSerializer.spec.js b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/markdownSerializer.spec.js index 552c8a6d75..ed2235b988 100644 --- a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/markdownSerializer.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/markdownSerializer.spec.js @@ -209,7 +209,7 @@ describe('createCustomMarkdownSerializer', () => { expect(getMarkdown()).toBe('*' + '**bold and italic**' + '*'); }); - it('should serialize an underlined node with a tag', () => { + it('should serialize an underlined node with __ delimiters (Perseus syntax)', () => { const docContent = [ { type: 'paragraph', @@ -224,7 +224,7 @@ describe('createCustomMarkdownSerializer', () => { ]; const mockEditor = createMockEditor(docContent); const getMarkdown = createCustomMarkdownSerializer(mockEditor); - expect(getMarkdown()).toBe('underlined'); + expect(getMarkdown()).toBe('__underlined__'); }); it('should serialize a link node correctly', () => { diff --git a/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/pasteTransform.spec.js b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/pasteTransform.spec.js new file mode 100644 index 0000000000..3f44d938cf --- /dev/null +++ b/contentcuration/contentcuration/frontend/shared/views/TipTapEditor/__tests__/pasteTransform.spec.js @@ -0,0 +1,136 @@ +import { transformPastedHTML } from '../TipTapEditor/utils/pasteTransform'; + +describe('transformPastedHTML', () => { + describe('empty inputs', () => { + it('returns empty string for empty input', () => { + expect(transformPastedHTML('')).toBe(''); + }); + + it('returns empty string for null', () => { + expect(transformPastedHTML(null)).toBe(''); + }); + + it('returns empty string for undefined', () => { + expect(transformPastedHTML(undefined)).toBe(''); + }); + }); + + describe('image stripping', () => { + it('strips a single remote img', () => { + const input = '

before after

'; + expect(transformPastedHTML(input)).toBe('

before after

'); + }); + + it('strips a data: URI img', () => { + const input = '

'; + expect(transformPastedHTML(input)).toBe('

'); + }); + + it('strips img with no src', () => { + const input = '

'; + expect(transformPastedHTML(input)).toBe('

'); + }); + + it.each([ + ['http', ''], + ['blob', ''], + ['file', ''], + ['relative', ''], + ])('strips img with %s scheme', (_scheme, imgTag) => { + expect(transformPastedHTML(`

${imgTag}

`)).toBe('

'); + }); + + it('strips multiple imgs in different parents', () => { + const input = [ + '

top

', + '', + '
  • item
', + ].join(''); + const output = transformPastedHTML(input); + expect(output).not.toContain('top

'); + expect(output).toContain('
  • item
'); + }); + + it('preserves surrounding marks when stripping mixed imgs', () => { + const input = + '

bold italic link

'; + const output = transformPastedHTML(input); + expect(output).not.toContain('bold'); + expect(output).toContain('italic'); + expect(output).toContain('link'); + }); + }); + + describe('Word/Office cleanup', () => { + it('removes MSO conditional comments', () => { + const input = '

before after

'; + expect(transformPastedHTML(input)).toBe('

before after

'); + }); + + it('removes Office-namespaced tags (w:, m:, o:, v:)', () => { + const input = + '

beforeafter

'; + const output = transformPastedHTML(input); + expect(output).not.toMatch(/<\/?[wmov]:/); + expect(output).toContain('before'); + expect(output).toContain('after'); + }); + + it('strips mso-* style declarations while keeping other styles', () => { + const input = + '

x

'; + const output = transformPastedHTML(input); + expect(output).not.toMatch(/mso-/); + expect(output).toContain('color: red'); + expect(output).toContain('font-size: 12pt'); + }); + + it('removes the style attribute entirely when all declarations were mso-*', () => { + const input = '

x

'; + expect(transformPastedHTML(input)).toBe('

x

'); + }); + + it('strips Mso* classes (case-insensitive) while keeping other classes', () => { + const input = '

x

'; + const output = transformPastedHTML(input); + expect(output).toContain('class="kept-class"'); + expect(output).not.toMatch(/Mso/i); + }); + + it('removes the class attribute entirely when all classes were Mso*', () => { + const input = '

x

'; + expect(transformPastedHTML(input)).toBe('

x

'); + }); + + it('hoists nested lists out of strike/s/del wrappers', () => { + const input = '
  • a
'; + const output = transformPastedHTML(input); + expect(output).toContain('
  • a
'); + expect(output.indexOf('')).toBeLessThan(output.indexOf('
    ')); + }); + + it('re-parents nested lists inside
  • to the end of the
  • ', () => { + const input = '
    • text
      • nested
      more text
    '; + const output = transformPastedHTML(input); + expect(output).toMatch(/
  • textmore text
    • nested<\/li><\/ul><\/li>/); + }); + }); + + describe('idempotency', () => { + it.each([ + ['

      plain text

      '], + ['

      before after

      '], + ['

      x

      '], + ['

      x

      '], + ['
      • a
      '], + ['
      • text
        • n
        more
      '], + ['

      y

      '], + ])('is idempotent: f(f(x)) === f(x) for %s', input => { + const once = transformPastedHTML(input); + const twice = transformPastedHTML(once); + expect(twice).toBe(once); + }); + }); +}); diff --git a/contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CompletionOptions/MasteryCriteriaGoal.vue b/contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CompletionOptions/MasteryCriteriaGoal.vue index 84e6b5b0fb..d3e2f70676 100644 --- a/contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CompletionOptions/MasteryCriteriaGoal.vue +++ b/contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CompletionOptions/MasteryCriteriaGoal.vue @@ -36,7 +36,10 @@ translateValidator, getInvalidText, } from 'shared/utils/validation'; - import MasteryModels, { MasteryModelsList } from 'shared/leUtils/MasteryModels'; + import MasteryModels, { + MasteryModelsList, + MasteryModelsNames, + } from 'shared/leUtils/MasteryModels'; import { constantsTranslationMixin } from 'shared/mixins'; import DropdownWrapper from 'shared/views/form/DropdownWrapper'; @@ -84,10 +87,13 @@ }, }, masteryCriteria() { - return MasteryModelsList.map(model => ({ - text: this.translateConstant(model), - value: model, - })); + // temporarily exclude pre/post test until the creation/editing UI is done + return MasteryModelsList.filter(model => model !== MasteryModelsNames.PRE_POST_TEST).map( + model => ({ + text: this.translateConstant(model), + value: model, + }), + ); }, masteryRules() { return this.required ? getMasteryModelValidators().map(translateValidator) : []; diff --git a/contentcuration/contentcuration/frontend/shared/views/contentNodeFields/__tests__/masteryCriteriaGoal.spec.js b/contentcuration/contentcuration/frontend/shared/views/contentNodeFields/__tests__/masteryCriteriaGoal.spec.js index 3c39caa45e..3567d8ce5c 100644 --- a/contentcuration/contentcuration/frontend/shared/views/contentNodeFields/__tests__/masteryCriteriaGoal.spec.js +++ b/contentcuration/contentcuration/frontend/shared/views/contentNodeFields/__tests__/masteryCriteriaGoal.spec.js @@ -2,7 +2,7 @@ import { mount } from '@vue/test-utils'; import MasteryCriteriaGoal from '../CompletionOptions/MasteryCriteriaGoal'; import TestForm from 'shared/views/__tests__/TestForm'; import { constantStrings } from 'shared/mixins'; -import MasteryModels from 'shared/leUtils/MasteryModels'; +import MasteryModels, { MasteryModelsNames } from 'shared/leUtils/MasteryModels'; describe('MasteryCriteriaGoal', () => {}); async function makeWrapper(propsData = {}) { @@ -28,9 +28,15 @@ describe('masteryCriteriaGoal', () => { describe('on load', () => { MasteryModels.forEach(model => { - it(`${model} mastery option should be an option to select`, () => { - expect(wrapper.findComponent('.v-list').text()).toContain(constantStrings.$tr(model)); - }); + if (model === MasteryModelsNames.PRE_POST_TEST) { + it(`${model} mastery option should not be an option to select (not yet implemented)`, () => { + expect(wrapper.findComponent('.v-list').text()).not.toContain(constantStrings.$tr(model)); + }); + } else { + it(`${model} mastery option should be an option to select`, () => { + expect(wrapper.findComponent('.v-list').text()).toContain(constantStrings.$tr(model)); + }); + } }); it('should render according to masteryModel prop', async () => { for (const model of MasteryModels) { diff --git a/contentcuration/contentcuration/tests/utils/test_urls.py b/contentcuration/contentcuration/tests/utils/test_urls.py new file mode 100644 index 0000000000..14a3e4095b --- /dev/null +++ b/contentcuration/contentcuration/tests/utils/test_urls.py @@ -0,0 +1,38 @@ +from django.contrib.sites.models import Site +from django.test import override_settings + +from contentcuration.tests.base import StudioTestCase +from contentcuration.utils.urls import canonical_url + + +class CanonicalUrlTestCase(StudioTestCase): + @override_settings(SITE_ID=1) + def test_production_domain_uses_https(self): + self.assertEqual( + canonical_url("/settings/"), + "https://studio.learningequality.org/settings/", + ) + + @override_settings(SITE_ID=3) + def test_branch_subdomain_uses_https(self): + self.assertEqual( + canonical_url("/settings/"), + "https://unstable.studio.learningequality.org/settings/", + ) + + @override_settings(SITE_ID=2) + def test_local_dev_loopback_uses_http(self): + self.assertEqual(canonical_url("/settings/"), "http://127.0.0.1:8080/settings/") + + def test_localhost_uses_http(self): + site = Site.objects.create( + pk=9999, domain="localhost:9000", name="Localhost test" + ) + with override_settings(SITE_ID=site.pk): + self.assertEqual( + canonical_url("/settings/"), "http://localhost:9000/settings/" + ) + + @override_settings(SITE_ID=1) + def test_empty_path_returns_bare_url(self): + self.assertEqual(canonical_url(), "https://studio.learningequality.org") diff --git a/contentcuration/contentcuration/tests/views/test_subscription.py b/contentcuration/contentcuration/tests/views/test_subscription.py index 889d3f4f58..1a21617dda 100644 --- a/contentcuration/contentcuration/tests/views/test_subscription.py +++ b/contentcuration/contentcuration/tests/views/test_subscription.py @@ -1,6 +1,7 @@ from unittest import mock import stripe +from django.contrib.sites.models import Site from django.test import override_settings from django.urls import reverse @@ -57,6 +58,33 @@ def test_rejects_user_with_active_subscription(self, mock_create): self.assertEqual(response.status_code, 400) mock_create.assert_not_called() + @override_settings(SITE_ID=1) + @mock.patch("contentcuration.views.subscription.stripe.checkout.Session.create") + def test_checkout_urls_use_canonical_site_domain(self, mock_create): + Site.objects.update_or_create( + pk=1, defaults={"domain": "studio.learningequality.org", "name": "Studio"} + ) + mock_create.return_value = mock.Mock(url="https://checkout.stripe.com/test") + + self.client.force_authenticate(self.user) + response = self.client.post( + self.url, + data={"storage_gb": 10}, + format="json", + HTTP_HOST="master.studio.learningequality.org", + ) + + self.assertEqual(response.status_code, 200) + call_kwargs = mock_create.call_args[1] + for key in ("success_url", "cancel_url"): + url = call_kwargs[key] + self.assertNotIn( + "master.studio.learningequality.org", + url, + f"{key} leaked internal hostname: {url}", + ) + self.assertIn("studio.learningequality.org", url) + @mock.patch("contentcuration.views.subscription.stripe.checkout.Session.create") def test_user_with_canceled_subscription_can_checkout_again(self, mock_create): """User whose subscription was canceled can create a new checkout session.""" @@ -124,6 +152,35 @@ def test_creates_portal_session(self, mock_create): data = response.json() self.assertEqual(data["portal_url"], "https://billing.stripe.com/test") + @override_settings(SITE_ID=1) + @mock.patch( + "contentcuration.views.subscription.stripe.billing_portal.Session.create" + ) + def test_portal_return_url_uses_canonical_site_domain(self, mock_create): + Site.objects.update_or_create( + pk=1, defaults={"domain": "studio.learningequality.org", "name": "Studio"} + ) + UserSubscription.objects.create( + user=self.user, + stripe_customer_id="cus_test123", + stripe_subscription_status="active", + ) + mock_create.return_value = mock.Mock(url="https://billing.stripe.com/test") + + self.client.force_authenticate(self.user) + response = self.client.post( + self.url, HTTP_HOST="master.studio.learningequality.org" + ) + + self.assertEqual(response.status_code, 200) + return_url = mock_create.call_args[1]["return_url"] + self.assertNotIn( + "master.studio.learningequality.org", + return_url, + f"return_url leaked internal hostname: {return_url}", + ) + self.assertIn("studio.learningequality.org", return_url) + @override_settings( STRIPE_SECRET_KEY="sk_test_fake", diff --git a/contentcuration/contentcuration/utils/urls.py b/contentcuration/contentcuration/utils/urls.py new file mode 100644 index 0000000000..7c2e7e5483 --- /dev/null +++ b/contentcuration/contentcuration/utils/urls.py @@ -0,0 +1,20 @@ +from django.contrib.sites.models import Site +from django.contrib.sites.shortcuts import get_current_site + +_LOCAL_HOSTS = ("127.0.0.1", "localhost", "::1") + + +def canonical_url(path="", request=None): + """Absolute URL on the Site framework's canonical domain. + + Prefer this over ``request.build_absolute_uri`` for URLs handed to + external systems: the latter reflects the Host header Django actually + received, which on production is the internal pod hostname rather + than the public canonical. + """ + if request is not None: + domain = get_current_site(request).domain + else: + domain = Site.objects.get_current().domain + scheme = "http" if domain.startswith(_LOCAL_HOSTS) else "https" + return f"{scheme}://{domain}{path}" diff --git a/contentcuration/contentcuration/views/subscription.py b/contentcuration/contentcuration/views/subscription.py index 784ba29798..2e87b76203 100644 --- a/contentcuration/contentcuration/views/subscription.py +++ b/contentcuration/contentcuration/views/subscription.py @@ -14,6 +14,7 @@ from contentcuration.models import User from contentcuration.models import UserSubscription +from contentcuration.utils.urls import canonical_url BYTES_PER_GB = 10 ** 9 MIN_STORAGE_GB = 1 @@ -52,10 +53,10 @@ def post(self, request): storage_gb = serializer.validated_data["storage_gb"] try: - success_url = request.build_absolute_uri( - "/settings/#/storage?upgrade=success" + success_url = canonical_url( + "/settings/#/storage?upgrade=success", request=request ) - cancel_url = request.build_absolute_uri("/settings/#/storage") + cancel_url = canonical_url("/settings/#/storage", request=request) checkout_session_params = { "mode": "subscription", @@ -79,9 +80,9 @@ def post(self, request): return Response({"checkout_url": session.url}) - except stripe.error.StripeError as e: - logger.error(f"Stripe error creating checkout session: {e}") - return Response({"error": str(e)}, status=400) + except stripe.error.StripeError: + logger.exception("Stripe error creating checkout session") + return Response({"error": "Unable to create checkout session"}, status=400) class CreatePortalSessionView(APIView): @@ -101,13 +102,13 @@ def post(self, request): try: session = stripe.billing_portal.Session.create( customer=subscription.stripe_customer_id, - return_url=request.build_absolute_uri("/settings/#/storage"), + return_url=canonical_url("/settings/#/storage", request=request), ) return Response({"portal_url": session.url}) - except stripe.error.StripeError as e: - logger.error(f"Stripe error creating portal session: {e}") - return Response({"error": str(e)}, status=400) + except stripe.error.StripeError: + logger.exception("Stripe error creating portal session") + return Response({"error": "Unable to create portal session"}, status=400) class SubscriptionStatusView(APIView): diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 46d6d063e8..6d2890d0be 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -468,8 +468,9 @@ def create(self, request, *args, **kwargs): try: self.perform_create(serializer) - except IntegrityError as e: - return Response({"error": str(e)}, status=409) + except IntegrityError: + logging.exception("Integrity error creating channel") + return Response({"error": "Channel could not be created"}, status=409) instance = serializer.instance Change.create_change( generate_create_event(