Project

General

Profile

Bug(バグ) #4055

OpenPNE が http://example.com/sns/ のようにサブディレクトリ以下に設置されている場合に、通知センターからアクセスする日記やコミュニティのURLが正しくない

Added by kaoru n almost 3 years ago. Updated over 1 year ago.

Status:
Pending Testing(テスト待ち)
Priority:
Normal(通常)
Target version:
Start date:
2016-11-21
Due date:
% Done:

70%

3.6 で発生するか:
No (いいえ)
3.8 で発生するか:
Yes (はい)

Description

Overview (現象)

OpenPNE が http://example.com/sns/ のようにサブディレクトリ以下に設置されている場合に、通知センターから日記等にアクセスする場合、http://example.com/sns/diary/1 のようなURLになるべきであるが、http://example.com/diary/1 にアクセスしてしまう。

Causes (原因)

通知センターに表示される通知のリンク先は、opNotificationCenter::notify()url パラメーターによって指定される。
このパラメーターを実装した当初は diary/show?id=1@diary_show?id=1 のような url_for ヘルパーに指定するための Internal URI の形式を想定していた。
しかし、実際には誤った実装によって Internal URI での指定は正しく機能していない状態となっていた。
(opJsonApiHelper が実行されるのは api アプリケーションであるため、url_for ではなく app_url_for を呼び出すのが正しい)

source:apps/api/lib/helper/opJsonApiHelper.php@8205623#L203

    'url' => $notification['url'] ? url_for($notification['url'], array('abstract' => true)) : null,

通知センターによる通知機能を提供するプラグイン等では、上記の不具合を避けるために Internal URI ではなく /diary/1 などの URI を直接出力することで目的の URI へのリンクを実現している。

しかし、/diary/1 などの URI を指定した場合には、本来 url_forapp_url_for であれば自動的に付加されるサブディレクトリ部分の相対パス (sfWebRequest::getRelativeUrlRoot() に相当) が付加されないため、当チケットで報告されているような OpenPNE をサブディレクトリに設置した場合のリンクの不具合が生じる。

Way to fix (修正内容)

JSON API の /api.php/push/search.json の実装を修正し、opNotificationCenter::notify()url パラメーターに Internal URI が指定された場合でも正しく動作するようにする。この場合は、API 実行時にサブディレクトリを考慮した URI が生成されるものとする。

また、/diary/1 のような URI を直接 url パラメーターに指定していたプラグイン等との互換性を保つため、先頭が / から始まる URI であった場合には app_url_for ヘルパーを介さずに直接文字列を出力するように対応する。
この場合は、API 側ではサブディレクトリ部分の付加は行わずに出力する。これは、opLikePlugin のようにプラグイン側で独自にサブディレクトリ部分を付加した URI を url パラメーターに指定している実装が存在しており、一律な対応を執ることが困難なためである。


Subtasks

Backport(バックポート) #4057: OpenPNE が http://example.com/sns/ のようにサブディレクトリ以下に設置されている場合に、通知センターからアクセスする日記やコミュニティのURLが正しくないInvalid(無効)isao sano

Backport(バックポート) #4058: OpenPNE が http://example.com/sns/ のようにサブディレクトリ以下に設置されている場合に、通知センターからアクセスする日記やコミュニティのURLが正しくないRejected(差し戻し)Youichi Kimura


Related issues

Related to opLikePlugin - Enhancement(機能追加・改善) #4064: OpenPNE が http://example.com/sns/ のようにサブディレクトリ以下に設置されている場合に、通知センターからアクセスするURLが正しくない問題に対する opLikePlugin の独自修正を取り除く Won't fix(対応せず) 2016-12-02

History

#1 Updated by kaoru n almost 3 years ago

  • Status changed from New(新規) to Accepted(着手)
  • Assignee set to kaoru n
  • Target version set to OpenPNE 3.9.0-old

#2 Updated by kaoru n almost 3 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

https://github.com/openpne/OpenPNE3/pull/393
にてプルリクエストしました。
レビューをお願いします。

#3 Updated by isao sano almost 3 years ago

  • Related to Backport(バックポート) #4057: OpenPNE が http://example.com/sns/ のようにサブディレクトリ以下に設置されている場合に、通知センターからアクセスする日記やコミュニティのURLが正しくない added

#4 Updated by isao sano almost 3 years ago

  • Related to Backport(バックポート) #4058: OpenPNE が http://example.com/sns/ のようにサブディレクトリ以下に設置されている場合に、通知センターからアクセスする日記やコミュニティのURLが正しくない added

#6 Updated by kaoru n over 2 years ago

  • Related to Enhancement(機能追加・改善) #4064: OpenPNE が http://example.com/sns/ のようにサブディレクトリ以下に設置されている場合に、通知センターからアクセスするURLが正しくない問題に対する opLikePlugin の独自修正を取り除く added

#7 Updated by Shinichi Urabe over 2 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)

ソースを見る限り openpne.baseUrl には http://xxx.xxx/sns のように HTTP スキームから始まるような URL が入ることがないとは思われるが
含まれた場合 http:/xxx.xxx/sns のように置換されてしまう、念のためその点も考慮したほうがよいと思われる

var linkUrl = 'http://example.com//test/test/test//test';
linkUrl.replace(/\/\//g, "/");

→ 結果: "http:/example.com/test/test/test/test"

必要とされて // 連続するスラッシュがURLに含まれる可能性があるか分からないが、 data-location-url や openpne.baseUrl に含められる文字列を過剰に置換する可能性があるのは懸念点となる
対応としては結合される data-location-url の先頭、openpne.baseUrl の末尾に一つだけ / が存在するようにしておくほうが望ましい

#8 Updated by kaoru n over 2 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

https://github.com/openpne/OpenPNE3/blob/stable-3.8.x/apps/pc_frontend/templates/_layout.php#L22
のように、openpne.baseUrl は末尾に '/' が一つ付与されるため、data-location-url の先頭の '/' を取り除いて文字列を連結するように プルリクエストを更新しました。
https://github.com/openpne/OpenPNE3/pull/393

#9 Updated by kaoru n over 2 years ago

  • 3.6 で発生するか changed from Unknown (未調査) to No (いいえ)
  • 3.8 で発生するか changed from Unknown (未調査) to Yes (はい)

#10 Updated by Shinichi Urabe over 2 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)

Javascript のコーディング規約はありませんが、少なくとも web/js/jquery.notify.js 内の文字リテラルは ' シングルクオートで囲んでいますので、統一しておいたほうがよいです
dataLocationUrl.replace(/^\/*/g, ""); の空文字のダブルクオートの箇所

#11 Updated by kaoru n over 2 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

変換文字列をダブルクォートではなくシングルクオートで囲むように修正しました
https://github.com/openpne/OpenPNE3/pull/393

#12 Updated by Shinichi Urabe over 2 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

#13 Updated by kaoru n over 2 years ago

  • Status changed from Pending Testing(テスト待ち) to Rejected(差し戻し)
  • % Done changed from 70 to 50

各プラグインにてサブディレクトリについて考慮されているものもあるため、下記の方針での修正とします。
・渡されたURLが「/」からはじまっている場合は、通知センター側でそのまま使用する、インターナルURIで渡された場合は、サブディレクトリ考慮するよう対応する
・プラグイン側は、現在サブディレクトリを考慮している場合はそのままでよい、考慮してないない場合は、インターナルURIで渡すように変更する

#14 Updated by kaoru n over 2 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

#4055-13 の仕様となるよう
https://github.com/openpne/OpenPNE3/pull/393
を更新しました。

渡されたURLが「/」からはじまっている場合のうち、「///」とスラッシュが連続していても通知センターでは考慮しません。
また、「http://」等から始まっている場合でも、「/」から始まっていないため、openpne.baseUrl を先頭に連結します。

#15 Updated by kaoru n over 2 years ago

各プラグインの状況

#16 Updated by Shinichi Urabe over 2 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

#17 Updated by Youichi Kimura over 2 years ago

  • Status changed from Pending Testing(テスト待ち) to Pending Review(レビュー待ち)
  • % Done changed from 70 to 50

#4055-13 についてですが、通知の URL に internal uri (@obj_member_profile?id=1member/1 のような形式) が使用された場合にサブディレクトリを考慮した URL を出力するというのは、JS 側ではなく API 側での対応を想定していました。
// 伝わりにくくてすみません...

以下の Pull Request が想定している修正内容です。
https://github.com/openpne/OpenPNE3/pull/433

各プラグインにおける修正との兼ね合いもあるため、この内容でプラグインが対応可能であるかもどうか念のため確認をお願いします。

#18 Updated by kaoru n over 2 years ago

  • Assignee changed from kaoru n to Youichi Kimura

#19 Updated by Shinichi Urabe over 2 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)
  • 課題本文の説明にある「原因」とそれに対する「修正内容」が以前の内容のままです
  • HTTPs:// のように URL スキームに大文字が混ざっている場合は想定しない動作になります

#20 Updated by Youichi Kimura over 2 years ago

  • Description updated (diff)
  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

#4055-19 の指摘について修正しました(master ブランチが変更になったため PR の扱いは要検討)
https://github.com/openpne/OpenPNE3/pull/433

#21 Updated by kaoru n over 2 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)
  • Target version changed from OpenPNE 3.9.0-old to OpenPNE 3.9.0

対象バージョン変更により修正内容の確認が必要であるため差し戻します。

#22 Updated by isao sano over 1 year ago

  • Status changed from Rejected(差し戻し) to Accepted(着手)
  • % Done changed from 50 to 0

#23 Updated by isao sano over 1 year ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

https://github.com/openpne/OpenPNE3/pull/502
にてプルリクエストしました。

#24 Updated by Rimpei Ogawa over 1 year ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

Also available in: Atom PDF